diff --git a/QuickFiler.Test/Controllers/EfcViewerTests.cs b/QuickFiler.Test/Controllers/EfcViewerTests.cs index be0718a56..89c4688cb 100644 --- a/QuickFiler.Test/Controllers/EfcViewerTests.cs +++ b/QuickFiler.Test/Controllers/EfcViewerTests.cs @@ -119,6 +119,24 @@ public void ClaimsAltChord_WithBareAltAndHandler_ReturnsTrue() .BeTrue("bare Alt is the chord the keyboard dialog services"); } + // Issue #726 finding 8: ClaimsAltChord's guard is `keyCode == Keys.Menu || keyCode == + // Keys.None` -- the bare-Alt test above only exercises the Keys.None disjunct (Keys.Alt + // carries no key-code bits). Keys.Menu | Keys.Alt is the VK_MENU keystroke shape (the Alt + // key itself pressed as a key) and exercises the other disjunct; without this test the + // Keys.Menu arm of the predicate was deletable without failing any existing test. + [TestMethod] + public void ClaimsAltChord_WithMenuKeyAndAlt_ReturnsTrue() + { + var handler = new Mock(); + + EfcViewer + .ClaimsAltChord(handler.Object, Keys.Menu | Keys.Alt) + .Should() + .BeTrue( + "Keys.Menu | Keys.Alt is the VK_MENU keystroke shape of the bare Alt chord" + ); + } + [TestMethod] public void ClaimsAltChord_WithAltF_ReturnsFalse() { diff --git a/QuickFiler.Test/Controllers/FilerQueueTests.cs b/QuickFiler.Test/Controllers/FilerQueueTests.cs index b7b56f1fd..b93dec733 100644 --- a/QuickFiler.Test/Controllers/FilerQueueTests.cs +++ b/QuickFiler.Test/Controllers/FilerQueueTests.cs @@ -97,9 +97,9 @@ private static TaskCompletionSource NewGate() => new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); /// - /// Enqueues one item. Every enqueued item carries a real helper because the preserved - /// worker catch block calls item.Helpers.First(); an empty list would raise - /// inside the catch, escape the worker loop, and leave the drain permanently incomplete. + /// Enqueues one item carrying a real helper, for tests that are not themselves about the + /// empty-helpers diagnostic path fixed under issue #726 (see + /// ). /// private static void EnqueueOne(FilerQueue queue) => queue.Enqueue(new EmailFiler(), OneHelper()); @@ -354,5 +354,76 @@ public async Task ItemProcessor_ThatThrows_StillDecrementsAndDrainCompletes() drain.IsCompleted.Should().BeTrue("the throwing item still decrements the counter"); invocations.Should().Be(2, "the worker loop continues past the failing item"); } + + [TestMethod] + public void Enqueue_NullItem_ThrowsArgumentNullException() + { + // Arrange + var queue = new FilerQueue(); + + // Act + Action act = () => queue.Enqueue((FilerQueueItem)null); + + // Assert + act.Should() + .Throw( + "issue #726 finding 1: a null item must never reach the queue -- it previously " + + "left the outstanding counter permanently incremented, since nothing was " + + "actually queued to decrement it" + ); + } + + /// + /// Regression test for issue #726 finding 1. Before the fix, an item constructed with an + /// empty (not null) helpers list -- which 's constructor + /// permits -- would cause item.Helpers.First() inside the worker's diagnostic catch + /// handler to throw . That exception escaped the + /// catch itself, unwound the entire worker loop, and left the consumer-running flag + /// permanently set with no try/finally to clear it -- so no later Enqueue + /// call would ever start a new worker again, permanently hanging the background mover. + /// + [TestMethod] + public async Task ConsumeAsync_ItemWithEmptyHelpersThrows_ConsumerRecoversForLaterItems() + { + // Arrange + var queue = new FilerQueue(); + var emptyHelpersItem = new FilerQueueItem(new EmailFiler(), new List()); + TaskCompletionSource secondProcessed = NewGate(); + int invocations = 0; + queue.ItemProcessor = item => + { + int index = Interlocked.Increment(ref invocations) - 1; + if (index == 0) + { + // Provoked by an EMPTY (not null) Helpers list, exactly like the diagnostic + // path's own item.Helpers.First() would throw before the fix. + throw new InvalidOperationException( + "processing fails for the empty-helpers item" + ); + } + + secondProcessed.TrySetResult(true); + return Task.CompletedTask; + }; + + // Act: enqueue the poisoning item, wait for the queue to fully drain (proving the + // worker did not hang), then enqueue a second, normal item and confirm a NEW worker + // starts and processes it -- which only happens if the consumer-running flag was + // actually cleared rather than left stuck by the first item's escaping exception. + queue.Enqueue(emptyHelpersItem); + await queue.WhenDrainedAsync(); + EnqueueOne(queue); + await secondProcessed.Task; + await queue.WhenDrainedAsync(); + + // Assert + invocations + .Should() + .Be( + 2, + "the consumer-running flag must clear on the exceptional exit path so a later " + + "Enqueue can start a fresh worker, not just on the normal empty-queue exit" + ); + } } } diff --git a/QuickFiler/Controllers/EfcFormController.cs b/QuickFiler/Controllers/EfcFormController.cs index b6ee2f5e6..ffc12116c 100644 --- a/QuickFiler/Controllers/EfcFormController.cs +++ b/QuickFiler/Controllers/EfcFormController.cs @@ -128,6 +128,33 @@ internal EfcFormController InitializeDataFields(EfcDataModel dataModel) internal System.Action BoundaryErrorSink { get; set; } = (message, exception) => logger.Error(message, exception); + /// + /// Issue #726 finding 5: invokes defensively so a null or + /// throwing sink delegate cannot silently reinstate the unobserved-fault behavior this + /// boundary exists to prevent -- these call sites all sit in an async void handler's + /// catch block, where an escaping exception would crash the process rather than merely fail + /// to log. + /// + private void TryReportBoundaryFault(string message, System.Exception exception) + { + var sink = BoundaryErrorSink; + if (sink is null) + { + logger.Error(message, exception); + return; + } + + try + { + sink(message, exception); + } + catch (System.Exception sinkException) + { + logger.Error($"{message} (and the error sink itself threw)", sinkException); + logger.Error(message, exception); + } + } + private IApplicationGlobals _globals; private System.Action _parentCleanup; private EfcDataModel _dataModel; @@ -453,7 +480,7 @@ internal async Task ButtonCancelClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -470,7 +497,7 @@ internal async Task ButtonOkClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -488,7 +515,7 @@ internal async Task ButtonRefreshClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -550,7 +577,7 @@ await _dataModel.MoveToFolderAsync( } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -565,7 +592,7 @@ internal async Task ButtonDeleteClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -1135,7 +1162,7 @@ public async Task PopulateFolderCombobox(object folderList = null) } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } diff --git a/QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs b/QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs new file mode 100644 index 000000000..76e7dd099 --- /dev/null +++ b/QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs @@ -0,0 +1,67 @@ +using System; +using System.Threading.Tasks; + +namespace QuickFiler.Controllers +{ + internal partial class EfcItemController + { + /// + /// Issue #726 finding 4: fault-boundary sink for , + /// mirroring QfcItemController.WebViewInitializationErrorSink. Named distinctly so no + /// shared contract with the QFC sink or with + /// is implied. + /// + internal Action WebViewInitializationErrorSink { get; set; } = + (message, exception) => logger.Error(message, exception); + + /// + /// Issue #726 finding 4: fault boundary for . Both + /// production call sites previously discarded the task returned by + /// Task.Run(() => InitializeWebViewAsync()), so a fault there was never observed -- + /// under .NET Framework 4.5+, a discarded faulted task is silently finalized with no + /// diagnostic. This member contains the fault instead of returning it: the task it returns + /// never transitions to Faulted. + /// + internal async Task InitializeWebViewGuardedAsync() + { + try + { + await InitializeWebViewAsync(); + } + catch (OperationCanceledException) + { + // Cooperative cancellation during teardown is expected and is not a fault. + } + catch (Exception ex) + { + // Issue #726 finding 5: guard against a null or throwing sink delegate so a + // misconfigured sink cannot silently reinstate the unobserved-fault behavior this + // boundary exists to prevent. + TryReportWebViewInitializationFault(ex); + } + } + + private void TryReportWebViewInitializationFault(Exception ex) + { + var sink = WebViewInitializationErrorSink; + if (sink is null) + { + logger.Error("WebView2 initialization failed.", ex); + return; + } + + try + { + sink("WebView2 initialization failed.", ex); + } + catch (Exception sinkException) + { + logger.Error( + "WebView2 initialization failed, and the error sink itself threw.", + sinkException + ); + logger.Error("Original WebView2 initialization failure.", ex); + } + } + } +} diff --git a/QuickFiler/Controllers/EfcItemController.cs b/QuickFiler/Controllers/EfcItemController.cs index d320b6395..37b14c1e5 100644 --- a/QuickFiler/Controllers/EfcItemController.cs +++ b/QuickFiler/Controllers/EfcItemController.cs @@ -23,7 +23,7 @@ namespace QuickFiler.Controllers { [ExcludeFromCodeCoverage] - internal class EfcItemController : IItemControler + internal partial class EfcItemController : IItemControler { #region Constructors and Initializers @@ -94,7 +94,9 @@ public EfcItemController InitializeDataFields(EfcDataModel dataModel) PopulateControls(dataModel); PopulateConversation(); WireEvents(); - Task.Run(() => InitializeWebViewAsync()); + // Issue #726 finding 4: routed through the guarded wrapper so a fault is logged + // instead of silently finalized away as an unobserved discarded-task exception. + _ = InitializeWebViewGuardedAsync(); return this; } @@ -150,7 +152,9 @@ private void Initialize(bool async) _itemPositionTips.Toggle(Enums.ToggleState.Off, shareColumn: true); WireEvents(); - Task.Run(() => InitializeWebViewAsync()); + // Issue #726 finding 4: routed through the guarded wrapper so a fault is logged + // instead of silently finalized away as an unobserved discarded-task exception. + _ = InitializeWebViewGuardedAsync(); } private static readonly log4net.ILog logger = log4net.LogManager.GetLogger( diff --git a/QuickFiler/Controllers/FilerQueue.cs b/QuickFiler/Controllers/FilerQueue.cs index d76508178..1f78af291 100644 --- a/QuickFiler/Controllers/FilerQueue.cs +++ b/QuickFiler/Controllers/FilerQueue.cs @@ -47,6 +47,8 @@ public class FilerQueue public void Enqueue(FilerQueueItem item) { + item.ThrowIfNull(); + bool startWorker; lock (_sync) @@ -117,46 +119,78 @@ public Task WhenDrainedAsync() } } - public async Task ConsumeAsync() + internal async Task ConsumeAsync() { await Task.Run(async () => { - while (true) + // Issue #726 finding 1: this outer try/finally is a safety net, not the primary + // mechanism. The normal exit path (TryTake fails) still clears the flag inside the + // same critical section as the failed take, which is what closes the orphaned-item + // window described below. This finally exists so that ANY exception escaping the + // loop -- including one from the diagnostic branch of the inner catch, which the + // inner catch cannot catch itself -- still clears the flag rather than leaving + // _consumerRunning permanently true and hanging the background mover. + try { - FilerQueueItem item; - - lock (_sync) + while (true) { - // Clearing the flag in the same critical section in which TryTake fails is what - // closes the orphaned-item window: a producer cannot observe "a worker is - // running" after this worker has decided to stop. - if (!Queue.TryTake(out item)) + FilerQueueItem item; + + lock (_sync) { - _consumerRunning = false; - return; + // Clearing the flag in the same critical section in which TryTake fails is + // what closes the orphaned-item window: a producer cannot observe "a worker + // is running" after this worker has decided to stop. + if (!Queue.TryTake(out item)) + { + _consumerRunning = false; + return; + } } - } - try - { - await ItemProcessor(item); - } - catch (Exception e) - { - var first = item.Helpers.First(); - logger.Error( - $"Error sorting mail items Subject: {first.Subject} Sent On: {first.SentOn} from {first.SenderName} {e.Message}", - e - ); + try + { + await ItemProcessor(item); + } + catch (Exception e) + { + LogItemFailure(item, e); + } + finally + { + CompleteItem(); + } } - finally + } + finally + { + lock (_sync) { - CompleteItem(); + _consumerRunning = false; } } }); } + /// + /// Issue #726 finding 1: Helpers may legitimately be empty (the constructor guards + /// against null and null elements, but not against an empty list), so the original + /// item.Helpers.First() could throw from + /// inside this catch handler -- an exception the catch cannot catch itself, which escaped + /// the worker loop entirely and left stuck. + /// + private void LogItemFailure(FilerQueueItem item, Exception e) + { + var first = item.Helpers?.FirstOrDefault(); + var subject = first?.Subject ?? "(no helpers)"; + var sentOn = first?.SentOn; + var sender = first?.SenderName ?? "(unknown)"; + logger.Error( + $"Error sorting mail items Subject: {subject} Sent On: {sentOn} from {sender} {e.Message}", + e + ); + } + /// /// Decrements the outstanding-work counter and, when it reaches zero, completes and clears the /// drain signal. The signal is captured under the monitor and completed outside it. diff --git a/QuickFiler/Controllers/QfcFormController.Actions.cs b/QuickFiler/Controllers/QfcFormController.Actions.cs index 7d57f3965..a69385bee 100644 --- a/QuickFiler/Controllers/QfcFormController.Actions.cs +++ b/QuickFiler/Controllers/QfcFormController.Actions.cs @@ -2,7 +2,6 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Threading; using System.Threading.Tasks; using System.Windows.Forms; @@ -43,9 +42,11 @@ listObjects is null return; } - _helperTasks = listObjects - .Select(x => MailItemHelper.FromMailItemAsync(x, _globals, Token, false)) - .ToList(); + // Issue #726 finding 6: this sync pipeline reaches QfcItemController.Initialize(false) + // -> PopulateControls(MailItem, int), which builds its own MailItemHelper via the + // synchronous constructor -- it never consumes a helper produced here. The prior + // per-item FromMailItemAsync call below was started, discarded unawaited, and its + // result read by nothing, so it was pure duplicated Outlook COM work. Removed. _groups = new QfcCollectionController( AppGlobals: _globals, viewerInstance: _formViewer, diff --git a/QuickFiler/Controllers/QfcFormController.cs b/QuickFiler/Controllers/QfcFormController.cs index b7fe4fd69..e06644692 100644 --- a/QuickFiler/Controllers/QfcFormController.cs +++ b/QuickFiler/Controllers/QfcFormController.cs @@ -89,7 +89,6 @@ public IQfcFormController Init() private Dictionary _themes; private BlockingCollection _undoQueue = []; private Task _undoConsumerTask; - private List> _helperTasks = []; #endregion diff --git a/QuickFiler/Controllers/QfcHomeController.Metrics.cs b/QuickFiler/Controllers/QfcHomeController.Metrics.cs index 38d33fdac..cf5e1cc77 100644 --- a/QuickFiler/Controllers/QfcHomeController.Metrics.cs +++ b/QuickFiler/Controllers/QfcHomeController.Metrics.cs @@ -151,6 +151,13 @@ public async Task WriteMetricsAsync(string filename) // If DebugLVL And vbCommand Then Debug.Print SubNm & " Variable durationText = " & durationText durationMinutesText = (Duration / 60d).ToString("##0.00", CultureInfo.InvariantCulture); + + // Issue #726 finding 2: GetMoveDiagnostics needs an AppointmentItem reference before it + // runs (it may annotate the appointment WriteMoveToCalendar creates), so the calendar + // write itself cannot move after the empty-diagnostics check below. What can move is + // the DECISION to keep the appointment: an empty-diagnostics session was previously + // producing a calendar entry with no corresponding metrics file. Deleting the orphaned + // appointment when there is nothing to report keeps the two artifacts symmetric. WriteMoveToCalendar( OlEndTime, OlStartTime, @@ -170,10 +177,17 @@ ref OlAppointment // The call is made through IQfcCollectionController.GetMoveDiagnostics, which carries // no XML documentation and therefore no non-null element guarantee, so this filter - // defends the interface contract rather than a known producer defect. - var lines = strOutput.Where(line => !string.IsNullOrWhiteSpace(line)).ToArray(); + // defends the interface contract rather than a known producer defect. Issue #726 + // finding 2: the array itself is also treated as possibly null for the same reason. + var lines = (strOutput ?? []).Where(line => !string.IsNullOrWhiteSpace(line)).ToArray(); if (lines.Length == 0) { + // Issue #726 finding 2: nothing to report, so the appointment WriteMoveToCalendar + // just created is deleted rather than left orphaned with no matching metrics file. + if (OlAppointment is not null) + { + OlAppointment.Delete(); + } return; } diff --git a/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs b/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs index e383e31b3..e798826de 100644 --- a/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs +++ b/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs @@ -34,7 +34,33 @@ internal async Task InitializeWebViewGuardedAsync() } catch (Exception ex) { - WebViewInitializationErrorSink("WebView2 initialization failed.", ex); + // Issue #726 finding 5: guard against a null or throwing sink delegate so a + // misconfigured sink cannot silently reinstate the unobserved-fault behavior this + // boundary exists to prevent. + TryReportWebViewInitializationFault(ex); + } + } + + private void TryReportWebViewInitializationFault(Exception ex) + { + var sink = WebViewInitializationErrorSink; + if (sink is null) + { + logger.Error("WebView2 initialization failed.", ex); + return; + } + + try + { + sink("WebView2 initialization failed.", ex); + } + catch (Exception sinkException) + { + logger.Error( + "WebView2 initialization failed, and the error sink itself threw.", + sinkException + ); + logger.Error("Original WebView2 initialization failure.", ex); } } } diff --git a/QuickFiler/QuickFiler.csproj b/QuickFiler/QuickFiler.csproj index 2f0782930..4f9fc6eee 100644 --- a/QuickFiler/QuickFiler.csproj +++ b/QuickFiler/QuickFiler.csproj @@ -301,6 +301,7 @@ + diff --git a/QuickFiler/Viewers/QfcFormViewer.cs b/QuickFiler/Viewers/QfcFormViewer.cs index 7bfac67be..53a5377d9 100644 --- a/QuickFiler/Viewers/QfcFormViewer.cs +++ b/QuickFiler/Viewers/QfcFormViewer.cs @@ -57,11 +57,11 @@ protected override bool ProcessCmdKey(ref Message msg, Keys keyData) { if (Controllers.QfcFormKeyHandler.ClaimsAltChord(_keyboardHandler, keyData)) { + // Issue #726 finding 8: the pre-existing `sender`/`e` locals from before this + // predicate was extracted were dead -- ToggleKeyboardDialogAsync() takes no + // arguments, and e.Handled had no effect since `e` was never wired to the real + // event pipeline. Removed rather than left as unrelated cruft. SynchronizationContext.SetSynchronizationContext(UiSyncContext); - object sender = FromHandle(msg.HWnd); - var e = new KeyEventArgs(keyData); - //_keyboardHandler.ToggleKeyboardDialog(sender, e); - e.Handled = true; _ = _keyboardHandler.ToggleKeyboardDialogAsync(); return true; } diff --git a/TaskVisualization/FlagChangeTrainingQueue.cs b/TaskVisualization/FlagChangeTrainingQueue.cs index b9e2c4693..2cb698809 100644 --- a/TaskVisualization/FlagChangeTrainingQueue.cs +++ b/TaskVisualization/FlagChangeTrainingQueue.cs @@ -39,21 +39,33 @@ internal async Task ConsumeAsync() await Task.Run( async () => { - while (Queue.TryTake(out var item)) + // Issue #726 finding 1: the identical handshake window that motivated the + // FilerQueue fix exists here too. The guard reset was the loop's last statement, + // unprotected by try/finally, so any exception escaping the loop -- including one + // from the catch handler's own diagnostic expression -- would leave _guard + // permanently in its already-fired state and stop Immediate-mode consumption from + // ever restarting. + try { - try + while (Queue.TryTake(out var item)) { - await item.ProcessGroupAsync(); - } - catch (Exception e) - { - logger.Error( - $"Error training flags for email with subject: {(item as FlagChangeGroup)?.Subject}. {e.Message}", - e - ); + try + { + await item.ProcessGroupAsync(); + } + catch (Exception e) + { + logger.Error( + $"Error training flags for email with subject: {(item as FlagChangeGroup)?.Subject}. {e.Message}", + e + ); + } } } - _guard = new ThreadSafeSingleShotGuard(); + finally + { + _guard = new ThreadSafeSingleShotGuard(); + } }, Cancel ); diff --git a/TaskVisualization/TaskViewer.cs b/TaskVisualization/TaskViewer.cs index 4c913de9a..77dd88f58 100644 --- a/TaskVisualization/TaskViewer.cs +++ b/TaskVisualization/TaskViewer.cs @@ -254,11 +254,19 @@ protected override bool ProcessCmdKey(ref Message msg, Keys keyData) { if (keyData.HasFlag(Keys.Alt)) { - // If keyData = Keys.Up OrElse keyData = Keys.Down OrElse keyData = Keys.Left OrElse keyData = Keys.Right OrElse keyData = Keys.Alt Then + // Issue #726 finding 7: the return value of KeyboardHandler_KeyDown was previously + // discarded and this method unconditionally returned true, claiming the entire Alt + // chord class regardless of whether the handler actually consumed the key -- the + // same over-claim shape already fixed for the QuickFiler EFC/QFC surfaces (#467, + // #663). Falling through to base.ProcessCmdKey when the handler does not consume the + // key restores standard WinForms menu-mnemonic routing for Alt chords it doesn't own. object sender = FromHandle(msg.HWnd); var e = new KeyEventArgs(keyData); - _controller.KeyboardHandler_KeyDown(sender, e); - return true; + bool consumed = _controller.KeyboardHandler_KeyDown(sender, e); + if (consumed) + { + return true; + } } return base.ProcessCmdKey(ref msg, keyData);