Repository navigation
Report the device search into the form only once there is a window - #23
Merged
Merged
Conversation
The v1.0.1 release failed again, on a different test in the same family: StatusBoxText_SetProperty_PropertiesCorrect asserted on the status box and found "Checking attached devices and updating port list..." in it. That is the device search writing to the form while the test was reading it. The previous fix let go of the form after each test, which stops a search outliving the test that started it. It could not help here, because this race is inside a single test: the form's own constructor starts the search, and the search then writes to the form the test is still using. The guard was the real fault. Every report the search makes decides how to marshal itself by asking the form, and a form that was never shown has no window handle. InvokeRequired on a handleless control answers false, which reads as "already on the right thread", so a pool thread writes the controls directly. The search now asks whether there is a window before reporting at all, in one place that all five of its calls into the form go through, and IGeneratorForm carries IsHandleCreated so it can be asked. That guard alone would have swapped one fault for a worse one. The search was started from the constructor, so on a quick machine it could finish before the window existed and have its results dropped, leaving the port list empty until a device was plugged or unplugged. It is started from OnHandleCreated now, which is the moment it becomes possible to report, so it can neither race the window nor be dropped. Tests never show a form, so they no longer start a search at all, which removes the race at its source rather than guarding it. Verified by running the suite six times over, and by watching the built application: it still announces the search on the status bar, which is the same guard the device list goes through, so both report as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w
There was a problem hiding this comment.
🟡 Changes recommended
OnHandleCreated can fire multiple times if the WinForms handle is recreated, so starting the device search unconditionally there risks repeated WMI searches/status churn and should be guarded to run once per form instance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a UI-thread affinity race in the device-search background thread by ensuring the search only reports into GeneratorForm once the form has a real window handle, avoiding cross-thread control updates during unit tests and early form lifetime.
Changes:
- Added
IGeneratorForm.IsHandleCreatedand used it inDeviceUpdateThreadto guard all UI reporting behind “parent + not terminating + handle exists”. - Moved the initial device search start from the form constructor to
GeneratorForm.OnHandleCreated, when reporting/marshalling becomes valid. - Updated
DeviceUpdateThreadtests to configure mockedIGeneratorForminstances as “handle created”.
File summaries
| File | Description |
|---|---|
GeneratorForm.cs |
Adds IsHandleCreated to IGeneratorForm and starts the device search from OnHandleCreated instead of the constructor. |
DeviceUpdateThread.cs |
Centralizes UI-reporting eligibility via ParentIsReady to prevent reporting into handleless forms. |
tests/RandomNumberGenerator.Test/DeviceUpdateThread.Test.cs |
Updates mocks to reflect the new “form must have a handle” requirement. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1356
to
+1360
| // The search runs on a pool thread and reports what it finds by calling this form, so it cannot | ||
| // start until there is a window for it to call. Started from the constructor it raced the window | ||
| // into being, and a search that got there first had its results dropped. | ||
| StartDeviceSearch(); | ||
| } |
Comment on lines
+135
to
+139
| // Stand in for a form that has been shown. The search only reports into a form whose window | ||
| // exists, because it decides how to marshal by asking the form, and a form with no window | ||
| // cannot answer that question. | ||
| mockParent.Setup(mock => mock.IsHandleCreated).Returns(true); | ||
|
|
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
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.
The v1.0.1 release failed a second time, on a different test in the same family.
StatusBoxText_SetProperty_PropertiesCorrectasserted on the status box and found "Checking attached devices and updating port list..." in it — the device search writing to the form while the test read it.Why the first fix could not have caught this
Clearing the parent after each test stops a search outliving the test that started it. This race is inside a single test: the form's own constructor starts the search, and the search then writes to the form the test is still using. Different shape, same root.
The actual fault
Every report the search makes decides how to marshal itself by asking the form. A form that was never shown has no window handle, and
InvokeRequiredon a handleless control answersfalse— which reads as "already on the right thread". So a pool thread writes the controls directly.The search now asks whether there is a window before reporting at all. All five of its calls into the form already shared one guard, so this is one place:
IGeneratorFormgainsIsHandleCreated, whichFormalready supplies fromControl.The regression that guard would have caused
Worth stating plainly, because it would have been worse than the flake: the search was started from the constructor. On a quick machine it could finish before the window existed, and the new guard would then have dropped its results — leaving the port list empty until a device was plugged or unplugged, with no retry.
So the search is started from
OnHandleCreatedinstead, the moment reporting becomes possible. It can now neither race the window nor be dropped. Tests never show a form, so they no longer start a search at all — which removes the race at its source rather than guarding against it.Verification
Checking attached devices and updating port list...). That message goes through the sameParentIsReadyguard as the device list, so seeing it proves both paths still report.check-nodevice23/23 against the built application.The eight
IGeneratorFormmocks in the device-thread tests now report a window, since they stand in for a form that has been shown.🤖 Generated with Claude Code
https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w