Skip to content

fix: make app shutdown disposal idempotent and ownership-correct#856

Merged
laurentiu021 merged 1 commit into
mainfrom
fix/r5-idempotent-dispose
Jun 11, 2026
Merged

fix: make app shutdown disposal idempotent and ownership-correct#856
laurentiu021 merged 1 commit into
mainfrom
fix/r5-idempotent-dispose

Conversation

@laurentiu021

Copy link
Copy Markdown
Owner

Summary

Makes the application's shutdown teardown idempotent and ownership-correct. App cleanup
ran 2–3 times on every exit; the redundant passes double-released the network charts'
unmanaged SkiaSharp paint/typeface handles. The resulting ObjectDisposedException was
caught and hidden (in App.OnExit and the global handler) but happened on every close.

Root cause

MainWindowViewModel.Dispose() is wired to two shutdown paths — the window's
OnClosed override and Application.Current.Exit (MainWindow.xaml.cs:54, 133) — and
the child VMs/services it disposes are DI singletons the ServiceProvider disposes
again on teardown (App.xaml.cs:92). Neither MainWindowViewModel nor
NetworkSharedState had a _disposed guard (they derive from ObservableObject, not
ViewModelBase, so they didn't inherit the existing guard), so the unmanaged paint
handles in NetworkSharedState.Dispose() were freed more than once — undefined behavior.

Additionally, NetworkSharedState.Dispose() called Pinger.Dispose() /
TraceMonitor.Dispose() on monitor services that are themselves DI singletons — an
ownership inversion (a consumer disposing a container-owned dependency).

Changes

  • NetworkSharedState: added a private bool _disposed; guard (if (_disposed) return; _disposed = true;) — same pattern as ViewModelBase/TrayIconService. Replaced Pinger.Dispose() / TraceMonitor.Dispose() with Pinger.Stop() / TraceMonitor.Stop() — stop the loops, let the container own disposal of the singletons. (It already called Stop() before Dispose(), so this just drops the redundant, ownership-inverting Dispose.)
  • MainWindowViewModel: added the same _disposed guard at the top of Dispose().
  • Test: Dispose_CalledTwice_DoesNotThrow in NetworkSharedStateTests pins the bug — a second Dispose() must be a no-op.

Why behavior is otherwise unchanged

The first Dispose() does exactly what it did before (minus the redundant singleton
Dispose that Stop() already covered). Child VMs derived from ViewModelBase were
already idempotent via their own guard; this adds the guard to the two ObservableObject
roots that lacked it. Runtime behavior on a single clean shutdown is identical; the
double/triple passes are now safe no-ops instead of hidden exceptions.

Verification

  • Build: main + Tests + IntegrationTests all 0 warnings / 0 errors.
  • Regression test compiles and pins the double-dispose scenario.
  • Self-review (Protocol I): leak/debug scan clean, author headers present, CHANGELOG +
    version bump (1.20.13 → 1.20.14) in this same PR.

fix: → patch release (1.20.14). Protocol C to follow after merge.

@laurentiu021 laurentiu021 merged commit 77f7d30 into main Jun 11, 2026
4 checks passed
@laurentiu021 laurentiu021 deleted the fix/r5-idempotent-dispose branch June 11, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant