Fix NetworkHelper re-entry guard and add Reset()#397
Conversation
- Scope re-entry guard to event-based setup path only, so token-based callers can retry after timeout/failure. - Add public `Reset()` to clear all state and allow reconfiguration from scratch. - Improve `AddressChangedCallback` to track IP loss and re-acquisition, resetting `NetworkReady` accordingly. - Add `Reconnecting` status enum value. - New tests covering retry-after-timeout and reset-then-restart scenarios. - Migrate all test assertions to TestFramework v3 API. - Update README with `NetworkHelper` usage guidance.
|
Warning Review limit reached
More reviews will be available in 27 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: nanoframework/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a public ChangesNetworkHelper reset and reconnection feature
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nanoFramework.System.Net/NetworkHelper/NetworkHelper.cs`:
- Around line 141-153: Reset() currently clears shared static state while
background worker (WorkingThread) and event handler (AddressChangedCallback) may
still be running; add a generation token or CancellationTokenSource that both
WorkingThread and AddressChangedCallback check and update atomically,
set/trigger cancellation and wait for the worker to exit (synchronize/Join/Wait)
before clearing fields, and only deregister AddressChangedCallback after
confirming callbacks are quiescent; update Reset(), WorkingThread(), and
AddressChangedCallback() to honor the token/cancellation and to avoid touching
shared fields when the generation/token has changed.
In `@nanoFramework.System.Net/NetworkHelper/NetworkHelperStatus.cs`:
- Around line 45-48: The new enum member Reconnecting was inserted before
ExceptionOccurred which changes ExceptionOccurred's numeric value; update
NetworkHelperStatus so existing numeric values are preserved by either moving
Reconnecting to the end of the enum declaration or by assigning explicit integer
values to all members (e.g., keep the original numeric value for
ExceptionOccurred) — adjust the enum definition (NetworkHelperStatus,
Reconnecting, ExceptionOccurred) accordingly to maintain wire compatibility for
consumers.
In `@README.md`:
- Line 66: Remove the stray bare Markdown heading "##" from the README (the
empty level-2 heading) by either deleting that line or replacing it with an
appropriate section title so the README renders correctly; search for the lone
"##" string in the file and update that location accordingly.
In `@Tests/NetworkHelperTests/ConnectToEthernetTests.cs`:
- Around line 104-110: The test currently allows a stale NetworkHelper.Status to
satisfy the assertion, so capture the pre-call state, call
NetworkHelper.SetupAndConnectNetwork(token: cs2.Token) into secondResult, and
then assert a concrete terminal outcome (e.g., secondResult indicates success or
timeout) instead of just Status != NetworkHelperStatus.None; alternatively
measure elapsed time with a Stopwatch around the second call to assert it waited
at least the expected timeout window (approx 10000ms minus small slack) and then
verify NetworkHelper.Status reflects the post-call terminal state. Use the
symbols NetworkHelper.SetupAndConnectNetwork, secondResult, cs2,
NetworkHelper.Status and NetworkHelperStatus.None to locate and update the
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2ed67c5-000a-4329-88b1-2282b3c501a9
📒 Files selected for processing (7)
README.mdTests/IPAddressTests/IPAddressTests.csTests/NetworkHelperTests/ConnectToEthernetTests.csTests/SocketTests/SocketExceptionsTests.csTests/SocketTests/SocketOptionsTests.csnanoFramework.System.Net/NetworkHelper/NetworkHelper.csnanoFramework.System.Net/NetworkHelper/NetworkHelperStatus.cs
af70432 to
338aab3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Description
Reset()to clear all state and allow reconfiguration from scratch.AddressChangedCallbackto track IP loss and re-acquisition, resettingNetworkReadyaccordingly.Reconnectingstatus enum value.NetworkHelperusage guidance.Motivation and Context
NetworkReadyremained permanently set after first connect, masking subsequent disconnections.How Has This Been Tested?
Screenshots
Types of changes
Checklist: