Restore OnError routing for NPC-sourced WhenPropertyChanged emissions; bump main to v10 preview#1
Closed
dwcullop wants to merge 1 commit into
Closed
Restore OnError routing for NPC-sourced WhenPropertyChanged emissions; bump main to v10 preview#1dwcullop wants to merge 1 commit into
dwcullop wants to merge 1 commit into
Conversation
…; bump main to v10 preview 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, do NOT swallow a throw from OnError itself: an unhandled downstream error (or a rethrowing error handler) still propagates, matching Subject<T>.OnNext semantics. Also bump version.json from 9.5-preview to 10.0-preview so main builds the next major preview. 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.
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.
Summary
PR reactivemarbles#1111 fixed a TOCTOU race in
WhenPropertyChanged/WhenValueChanged, but a follow-up commit in that PR stopped routing downstream exceptions throughOnError. Because these emissions originate from anINotifyPropertyChangedcallback, an unhandledOnNextthrow escaped raw out of the property setter that raisedPropertyChanged. 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.EmitCurrentandDeepChainSubscription.ProcessSignalagain wrap the accessor read, chain walk, and the downstreamOnNextin try/catch, routing any exception throughOnError. A subscriber that supplied an error handler now observes the failure instead of it escaping into the setter.OnErroritself. An unhandled downstream error (or a rethrowing error handler) still propagates, matchingSubject<T>.OnNextsemantics. This is the middle ground between the original swallow-everything behavior and the raw-escape behavior that shipped.version.json:9.5-preview->10.0-previewsomainbuilds the next major preview.Behavior
OnNextthrows, has error handlerOnError, setter safeOnNextthrows, no error handlerOnError(no-op after detach), setter safeTests
WhenPropertyChangedBehaviorFixturegains 4 tests (shallow + deep) covering theOnNextrouting restore (RED on current main) and the non-swallow decision. FullBindingsuite: 156 passed, 2 pre-existing skips.Labels / breaking change
manual-version-edit: required bypr-version-check.ymlfor a human edit toversion.json.breaking-change: the major bump 9 -> 10, and the error-propagation behavior change, are both observable. Flagging so maintainers make the call.