monitor: deregister after side effects; never join holding the lock - #153
Merged
Conversation
Two lifecycle defects found during adversarial review of the delayed-connect test flake (see #152): - monitor_loop removed its registry entries BEFORE running the dhcp/route/ autostart phase (and early-return paths skipped cleanup entirely), so a thread could be invisible to stop helpers while side effects were still pending. Cleanup now runs in a finally after the body, on every path. - stop_connection_monitor and stop_all_connection_monitors held _monitor_lock across join(), but the monitor thread's own deregistration needs that lock, guaranteeing the join times out in exactly the window it matters. Joins now happen outside the lock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Product-code follow-up from the #152 adversarial review. Two ConnectionMonitor lifecycle defects:
Deregistration before side effects.
monitor_looppopped its registry entries before running the dhcp/route/autostart phase, and the early-return paths (stop event set, missing stop event) skipped cleanup entirely. A thread could be invisible tostop_*helpers while its side effects were still pending, which is what let patch teardown race the action phase in the test flake. Cleanup now runs in afinallyaround the whole body: deregistration happens after all side effects, on every exit path.Join while holding
_monitor_lock. Both stop helpers joined threads while holding the lock that the monitor thread's own deregistration needs, guaranteeing a 2s join timeout in exactly the window where the thread is finishing. Joins now happen outside the lock; registry mutation reacquires it afterward.No API changes. The #152 test harness (Event-synchronized waits, liveness-checking idle helper) exercises these paths.