From 3f9ed44d6c7674d29eed38c5ffefdcf10364ebfe Mon Sep 17 00:00:00 2001 From: Skorinn <42702903+Skorinn@users.noreply.github.com> Date: Wed, 9 Sep 2026 07:40:49 -0500 Subject: [PATCH] Report the device search into the form only once there is a window 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) Claude-Session: https://claude.ai/code/session_0153VVkWg7DQmdaNLcvtY37w --- DeviceUpdateThread.cs | 34 +++++++--- GeneratorForm.cs | 66 ++++++++++++++----- .../DeviceUpdateThread.Test.cs | 40 +++++++++++ 3 files changed, 115 insertions(+), 25 deletions(-) diff --git a/DeviceUpdateThread.cs b/DeviceUpdateThread.cs index e70bed4..26c6052 100644 --- a/DeviceUpdateThread.cs +++ b/DeviceUpdateThread.cs @@ -14,6 +14,8 @@ // 2026/09/07 - Mike Pullen - Report device status through the shared status palette // 2026/09/09 - Mike Pullen - Read the severity colours where they are shown rather than holding them, so a // scheme changed while running is followed +// 2026/09/09 - Mike Pullen - Reported the search into the form only once its window exists, as InvokeRequired +// cannot say which thread owns a form that was never shown //********************************************************************************************************************* using System; using System.ComponentModel; @@ -144,7 +146,7 @@ private static void GetDevicePorts() private static void UpdateDeviceList() { // Check if the parent has been set and not terminating early - if ((null != m_Parent) && (false == Terminating)) + if (ParentIsReady) { // Check if invoke is required (should be) if (m_Parent.InvokeRequired) @@ -165,8 +167,8 @@ private static void UpdateDeviceList() /// private static void BackupInfoBox() { - // Make sure the parent is valid and not terminating early - if ((null != m_Parent) && (false == Terminating)) + // Make sure there is a window to report into + if (ParentIsReady) { // Get the current status box state through the parent's interface m_Parent.GetStatusBoxState(out m_sStatusBoxText, out m_StatusBoxTextColor, out m_StatusBoxBackColor); @@ -181,8 +183,8 @@ private static void BackupInfoBox() /// IN - Background color to set for the info box private static void UpdateInfoBox(string sText, Color textColor, Color backColor) { - // Make sure the parent is valid and not terminating early - if ((null != m_Parent) && (false == Terminating)) + // Make sure there is a window to report into + if (ParentIsReady) { // Use parent's method which already handles invoke requirements m_Parent.SetStatusBoxState(sText, textColor, backColor); @@ -197,8 +199,8 @@ private static void UpdateInfoBox(string sText, Color textColor, Color backColor /// private static void RestoreInfoBox() { - // Make sure the parent is valid and not terminating early - if ((null != m_Parent) && (false == Terminating)) + // Make sure there is a window to report into + if (ParentIsReady) { // Get the message currently displayed in the info box string sCurrentText; @@ -225,8 +227,8 @@ private static void RestoreInfoBox() /// IN - The failure that stopped the search private static void ReportDeviceUpdateFailure(Exception deviceException) { - // Make sure the parent is valid and not terminating early - if ((null != m_Parent) && (false == Terminating)) + // Make sure there is a window to report into + if (ParentIsReady) { string sMessage = $"{m_sREADING_DEVICES_ERROR} {deviceException.Message}"; m_Parent.SetStatusBoxState(sMessage, StatusPalette.ErrorText, StatusPalette.ErrorBackground); @@ -248,6 +250,20 @@ private static void ReportDeviceUpdateFailure(Exception deviceException) /// public static bool Terminating { get => (null != m_Parent) && (GeneratorForm.RngGuiStates.Terminating == m_Parent.State); } + /// + /// Whether there is a window to report the search into. A parent has to be set, it has to not be + /// closing, and its window has to exist: the search runs on a pool thread and reports by calling the + /// form, and every one of those calls decides how to marshal itself by asking InvokeRequired. + /// A form that has never been shown has no handle, and InvokeRequired on a handleless control + /// answers false, which reads as "already on the right thread" - so the search writes the controls + /// from the pool thread and races whoever else is using them. Nothing is displaying a form that has + /// no window, so there is nothing to report to and the result is dropped rather than forced in. + /// + private static bool ParentIsReady + { + get => ((null != m_Parent) && (false == Terminating) && m_Parent.IsHandleCreated); + } + // Synchronizaion objects private static object m_Lock = new object(); diff --git a/GeneratorForm.cs b/GeneratorForm.cs index f9fbf7b..319ec84 100644 --- a/GeneratorForm.cs +++ b/GeneratorForm.cs @@ -46,6 +46,13 @@ public interface IGeneratorForm : ISynchronizeInvoke { BindingList DeviceList { set; } bool FileBrowseActive { get; set; } + + // Whether the window exists yet. Background work has to ask before reporting into the form, because + // InvokeRequired cannot answer for a form that has never been shown: with no window handle there is + // no thread to compare against, so it says false, which reads as "already on the right thread" and + // is how a pool thread ends up writing controls directly. Form supplies this from Control. + bool IsHandleCreated { get; } + bool Running { get; } Color StatusBoxBackColor { get; set; } string StatusBoxText { get; set; } @@ -143,22 +150,11 @@ public GeneratorForm(IRNGSessionData sessionData, IRNGDeviceTimer timer) m_StatusLabel.BackColor = StatusPalette.NormalBackground; m_StatusIndicator.ForeColor = StatusPalette.NormalText; - // Start the device update thread and trigger an update - DeviceUpdateThread.Parent = this; - ThreadPool.QueueUserWorkItem(state => - { - m_DeviceUpdateComplete.Reset(); // Clear the device update complete flag - try - { - DeviceUpdateThread.ThreadProc(state); - } - finally - { - // Signal that the device update has finished however it ended, so a failure does not - // leave the close waiting for an update that will never report itself complete - m_DeviceUpdateComplete.Set(); - } - }); + // The device search is not started here. It reports what it finds by calling this form, and the + // form cannot be called until its window exists, so it is started from OnHandleCreated instead. + // Starting it here raced the window into being: a search that finished first had nowhere to + // report to and its results were dropped, leaving the port list empty until a device was + // plugged or unplugged. // Create the source from the list of device ports m_DeviceBindingSource = new BindingSource(); @@ -970,6 +966,30 @@ private void OnDataPointAdded(double fDataPoint, double fCurrentAverage) } } + /// + /// Points the device search at this form and runs one, on the thread pool so the window is not held + /// while WMI is asked what is attached. Called once the window exists, because that is what the + /// search needs in order to report back. + /// + private void StartDeviceSearch() + { + DeviceUpdateThread.Parent = this; + ThreadPool.QueueUserWorkItem(state => + { + m_DeviceUpdateComplete.Reset(); // Clear the device update complete flag + try + { + DeviceUpdateThread.ThreadProc(state); + } + finally + { + // Signal that the device update has finished however it ended, so a failure does not + // leave the close waiting for an update that will never report itself complete + m_DeviceUpdateComplete.Set(); + } + }); + } + /// /// Initializes the thread pool /// @@ -1325,6 +1345,20 @@ private void ApplyTheme() m_ResultHistogramChart.ApplyPalette(); } + /// + /// Event handler for the window being created, which is when the device search can first be run + /// + /// IN - The event arguments (not used) + protected override void OnHandleCreated(EventArgs e) + { + base.OnHandleCreated(e); + + // 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(); + } + /// /// Event handler for the system colour scheme being changed while the application is running /// diff --git a/tests/RandomNumberGenerator.Test/DeviceUpdateThread.Test.cs b/tests/RandomNumberGenerator.Test/DeviceUpdateThread.Test.cs index 60c16ca..7b1782a 100644 --- a/tests/RandomNumberGenerator.Test/DeviceUpdateThread.Test.cs +++ b/tests/RandomNumberGenerator.Test/DeviceUpdateThread.Test.cs @@ -132,6 +132,11 @@ public void Parent_SetAndGetProperty_WorksCorrectly() // Create mock parent form var mockParent = new Mock(); + // 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); + //**************************************************************// // Act //**************************************************************// @@ -163,6 +168,11 @@ public void Terminating_ParentNotTerminating_ReturnsFalse() // Create mock parent form with non-terminating state var mockParent = new Mock(); + + // 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); mockParent.Setup(mock => mock.State).Returns(GeneratorForm.RngGuiStates.Idle); // Set the parent @@ -196,6 +206,11 @@ public void Terminating_ParentTerminating_ReturnsTrue() // Create mock parent form with terminating state var mockParent = new Mock(); + + // 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); mockParent.Setup(mock => mock.State).Returns(GeneratorForm.RngGuiStates.Terminating); // Set the parent @@ -258,6 +273,11 @@ public void ThreadProc_ValidParent_UpdatesDeviceListAndManagesStatusBox() // Create mock parent form var mockParent = new Mock(); + + // 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); mockParent.Setup(mock => mock.State).Returns(GeneratorForm.RngGuiStates.Idle); mockParent.Setup(mock => mock.InvokeRequired).Returns(true); mockParent.Setup(mock => mock.Invoke(It.IsAny())).Callback(action => action()); @@ -346,6 +366,11 @@ public void ThreadProc_ParentNoInvokeRequired_UpdatesDirectly() // Create mock parent form that doesn't require invoke var mockParent = new Mock(); + + // 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); mockParent.Setup(mock => mock.State).Returns(GeneratorForm.RngGuiStates.Idle); mockParent.Setup(mock => mock.InvokeRequired).Returns(false); @@ -398,6 +423,11 @@ public void ThreadProc_ConcurrentCalls_RespectsSynchronization() // Create mock parent form var mockParent = new Mock(); + + // 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); mockParent.Setup(mock => mock.State).Returns(GeneratorForm.RngGuiStates.Idle); mockParent.Setup(mock => mock.InvokeRequired).Returns(false); @@ -491,6 +521,11 @@ public void ThreadProc_InfoBoxUnchanged_RestoresPreviousMessage() // Create mock parent form var mockParent = new Mock(); + + // 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); mockParent.Setup(mock => mock.State).Returns(GeneratorForm.RngGuiStates.Idle); mockParent.Setup(mock => mock.InvokeRequired).Returns(false); @@ -550,6 +585,11 @@ public void ThreadProc_InfoBoxChangedDuringUpdate_NewerMessageKept() // Create mock parent form var mockParent = new Mock(); + + // 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); mockParent.Setup(mock => mock.State).Returns(GeneratorForm.RngGuiStates.Idle); mockParent.Setup(mock => mock.InvokeRequired).Returns(false);