Skip to content

Restore OnError routing for NPC-sourced WhenPropertyChanged emissions#1127

Open
dwcullop wants to merge 1 commit into
reactivemarbles:mainfrom
dwcullop:bugfix/restore-whenpropertychanged-onerror
Open

Restore OnError routing for NPC-sourced WhenPropertyChanged emissions#1127
dwcullop wants to merge 1 commit into
reactivemarbles:mainfrom
dwcullop:bugfix/restore-whenpropertychanged-onerror

Conversation

@dwcullop

@dwcullop dwcullop commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

PR #1111 fixed a TOCTOU race in WhenPropertyChanged / WhenValueChanged, but a follow-up commit in that PR stopped routing downstream exceptions through OnError. Because these emissions originate from an INotifyPropertyChanged callback, an unhandled OnNext throw escaped raw out of the property setter that raised PropertyChanged: setting a property could throw because some unrelated observer misbehaved.

This restores the error routing on both the shallow (x => x.Prop) and deep-chain (x => x.A.B.Prop) paths.

What changed

  • SinglePropertySubscription.EmitCurrent and DeepChainSubscription.ProcessSignal again wrap the accessor read, chain walk, and downstream OnNext in try/catch, routing any exception through OnError. A subscriber that supplied an error handler now observes the failure instead of it escaping into the setter.
  • Per @JakenVeina's review point on WhenPropertyChanged: don't drop events fired during subscribe #1111, we deliberately do not swallow a throw from OnError itself. An unhandled downstream error (or a rethrowing error handler) still propagates, matching Subject<T>.OnNext semantics. This is the middle ground between the original swallow-everything behavior and the raw-escape behavior that shipped.

Behavior

Scenario Current main After
Subscriber OnNext throws, has error handler escapes raw to setter, handler bypassed routed to OnError, setter safe
Subscriber OnNext throws, no error handler escapes to setter routed to OnError (no-op after detach), setter safe
Accessor throws, error handler rethrows propagates to setter propagates to setter (unchanged)

Tests

WhenPropertyChangedBehaviorFixture gains 4 tests (shallow + deep) covering the OnNext-routing restore (RED on current main) and the non-swallow decision. Full Binding suite: 156 passed, 2 pre-existing skips.

PR reactivemarbles#1111 (WhenPropertyChanged race fix) rewrote the shallow and deep-chain observation paths and, in a follow-up commit, stopped routing downstream exceptions through OnError. Because these emissions originate from an INotifyPropertyChanged callback, an unhandled OnNext throw escaped raw out of the property setter that raised PropertyChanged.

Restore the try/catch that routes accessor, chain-walk, and downstream OnNext exceptions through OnError so a subscriber that supplied an error handler observes the failure and the setter stays safe. Per Jake's review point on reactivemarbles#1111, do NOT swallow a throw from OnError itself: an unhandled downstream error (or a rethrowing error handler) still propagates, matching Subject<T>.OnNext semantics.

Adds regression tests in WhenPropertyChangedBehaviorFixture covering both the OnNext-routing restore (RED on current main) and the non-swallow decision, for the shallow and deep-chain paths.
@dwcullop dwcullop added the bug label Jul 2, 2026
@dwcullop dwcullop marked this pull request as ready for review July 2, 2026 15:24
@JakenVeina

Copy link
Copy Markdown
Collaborator

This will only make the described scenario worse. Instead of getting thrown out of the property setter, the exception will now infinite loop through the stream chain, and hang the entire thread. Throwing exceptions upstream is something that only happens when there is no downstream exception handler, or when there is a defect within an operator.

If a consumer can't tolerate exceptions bubbling out of a property setter, they need to introduce proper handling for it, either downstream, or just below the .WhenPropertyChanged() subscription. We can introduce an overload of .WhenPropertyChanged() that allows an error handler to be supplied, and/or we can add an operator for catching downstream errors. Maybe even suggest such an operator to dotnet/reactive, I don't think there's an existing one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants