fix(reactive): treat a disposed dispatcher provider as no dispatcher - #3175
Open
nickrandolph wants to merge 2 commits into
Open
fix(reactive): treat a disposed dispatcher provider as no dispatcher#3175nickrandolph wants to merge 2 commits into
nickrandolph wants to merge 2 commits into
Conversation
Removing an event handler from a BindableCollection resolves the current dispatcher so it can find the thread's DataLayer, and that resolution reads DispatcherQueueProvider's static ThreadLocal<IDispatcher>. ThreadLocal disposes itself from its own finalizer, so once the assembly owning that static is unloaded with a collectible AssemblyLoadContext the read starts throwing ObjectDisposedException. Removal is reached FROM a finalizer (~ItemsSourceView disposes its collection-changed listener, which unsubscribes), and an exception escaping a finalizer is unrecoverable: it terminates the process rather than failing one operation. - DispatcherLocal routes every current-dispatcher resolution through FindCurrentDispatcher, which reports null on ObjectDisposedException. Null is the truthful answer, not a degraded one: a finalizer thread is not a UI thread and has no dispatcher to find, so the existing background-value path is correct for it. allowBackgroundValue is never overridden in this repo (default true), so that path is always available. - DispatcherQueueProvider.GetForCurrentThread guards the same way, so the provider cannot throw even for a consumer that resolves it directly. - Only ObjectDisposedException is treated this way. Any other failure to resolve a dispatcher stays fatal, or a misconfigured provider would silently degrade every consumer to background values -- asserted by a test. Fixes #3174 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is narrowly scoped, has targeted unit coverage, and only leaves minor doc wording nits to address.
Pull request overview
Addresses a crash scenario in the Reactive dispatching layer when the current-dispatcher provider has been disposed (notably when Uno.Extensions.Reactive.UI is loaded/unloaded via a collectible AssemblyLoadContext), by treating ObjectDisposedException during dispatcher resolution as “no dispatcher” and falling back to background-value behavior.
Changes:
- Route all current-dispatcher resolution in
DispatcherLocal<T>through a newFindCurrentDispatcher()helper that returnsnullonObjectDisposedException. - Add the same
ObjectDisposedException→nullguard toDispatcherQueueProvider.GetForCurrentThread()for defense in depth. - Add unit tests proving
ObjectDisposedExceptionfalls back safely while other provider failures still propagate.
File summaries
| File | Description |
|---|---|
| src/Uno.Extensions.Reactive/Utils/Dispatching/DispatcherLocal.cs | Adds FindCurrentDispatcher() and uses it for all current-dispatcher lookups to avoid ObjectDisposedException escaping finalizer-driven code paths. |
| src/Uno.Extensions.Reactive.UI/Utils/Dispatching/DispatcherQueueProvider.cs | Guards ThreadLocal access so GetForCurrentThread() returns null instead of throwing after disposal. |
| src/Uno.Extensions.Reactive.Tests/Utils/Dispatching/Given_DispatcherLocal.cs | Adds regression tests covering disposed-provider fallback and “other exceptions still throw” behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Both remarks blocks said a static becomes "collectable"; the runtime's own term is "collectible" (AssemblyLoadContext.IsCollectible), which is what a reader will search for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Fixes #3174 — read that issue for the full mechanism; this is the fix and its evidence.
Draft because the reproduction that motivated it is an end-to-end collectible-
AssemblyLoadContexthost that lives downstream, so CI here can only prove the unit-level behaviour. See Verification for exactly what is and is not established.The bug in one paragraph
Removing an event handler from a
BindableCollectionresolves the current dispatcher so it can find that thread'sDataLayer(BindableCollection.cs:239→DispatcherLocal.Value→_schedulersProvider()), and that resolution readsDispatcherQueueProvider's staticThreadLocal<IDispatcher>.ThreadLocal<T>disposes itself from its own finalizer, so once the assembly owning that static is unloaded with a collectible ALC, the read throwsObjectDisposedException. Removal is reached from a finalizer —~ItemsSourceView()disposes its collection-changed listener, which unsubscribes — and an exception escaping a finalizer is unrecoverable: it terminates the process.The change
DispatcherLocal(core) — every current-dispatcher resolution now goes throughFindCurrentDispatcher(), which reportsnullonObjectDisposedException. All four call sites are covered:Valueget,Valueset,GetValue,TryGetValue. (TryGetValuematters even though the owner is passed explicitly — it still reads the current dispatcher to decide whether creation from another thread is permitted.)Null is the truthful answer here, not a degraded one: a finalizer thread is not a UI thread, so there is genuinely no dispatcher to find.
DispatcherLocalalready has the background-value path for exactly that case, andallowBackgroundValueis never overridden anywhere in this repo (defaulttrue,DispatcherLocal.cs:41; the only production construction isBindableCollection.cs:132, which passes onlyallowCreationFromAnotherThread: true) — so that path is always available. Had it beenfalse, this fix would merely have swapped a fatalObjectDisposedExceptionfor a fatalInvalidOperationException, which is why it is called out rather than assumed.DispatcherQueueProvider(UI) —GetForCurrentThread()guards the same way, so the provider cannot throw even for a consumer that resolves it directly rather than throughDispatcherLocal. Defence in depth; the core fix is the load-bearing one.Only
ObjectDisposedExceptionis treated this way. Any other resolution failure stays fatal — a misconfigured provider must not silently degrade every consumer to background values. There is a test for that specifically.Chosen this way because
DispatcherLocalis the layer that depends on "resolve the current dispatcher", it is reachable from every one of the eight removal paths inBindableCollection(lines 239, 245, 335, 338, 344, 347, 353, 356 —VectorChangedand the selection handlers resolve on removal too, soCollectionChangedis not the only exposure), and it takes an injectable provider, which makes the fix testable without disposing a process-wide static from a test.Deliberately not done here
Removal should arguably not resolve a dispatcher at all.
DispatcherLocal.ForEachValuealready iterates existing layers without resolving or creating anything, so the removal accessors could apply across all layers and be naturally idempotent — which would additionally fix removing a handler from a different thread than the one that added it, and would avoid constructing aDataLayeron a finalizer thread.That is a behavioural change to event-removal semantics across eight accessors, including token-based overloads whose tolerance for unknown tokens I have not audited. It does not belong on a servicing branch. Happy to open it against
mainif you want it.Verification
Red/green, run both ways:
Given_DispatcherLocal)DispatcherLocal.csreverted, tests keptWhen_ProviderDisposed_Then_ValueFallsBackToBackground,When_ProviderDisposed_Then_TryGetValueDoesNotThrowUno.Extensions.Reactive.Testsdotnet buildof the test projectThe third new test —
When_ProviderThrowsOtherError_Then_ItPropagates— passes in both directions by design: it asserts the exception types that must keep throwing, so it guards the narrowness of thecatchrather than the fix itself.Not established here, and worth being plain about: no end-to-end proof that the process no longer dies. That needs a host loading
Uno.Extensions.Reactive.UIinto a collectible ALC, unloading it, and forcing finalizers — the setup described in the issue. The unit tests reproduce the provider state that causes it (a provider that throwsObjectDisposedException) and prove the resolution path now tolerates it; they do not reproduce the ALC teardown itself. I will confirm end-to-end against a downstream host once this is in a 7.4-dev package and report back on the issue.One compile detail, in case it comes up in review: the XML doc on
FindCurrentDispatcherrefers toDispatcherQueueProviderin prose rather than with acref, because it lives in the UI assembly and core does not reference it — acreffails the build with CS1574.