-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Observable): Another attempt at fixing error rethrows in v5 #3161
Conversation
Generated by 🚫 dangerJS |
@@ -68,6 +68,7 @@ export class Subscriber<T> extends Subscription implements Observer<T> { | |||
} | |||
if (typeof destinationOrNext === 'object') { | |||
if (destinationOrNext instanceof Subscriber) { | |||
this.syncErrorThrowable = destinationOrNext.syncErrorThrowable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one in particular I'm weary about, but for reasons unknown to me it's the only way the other change works in every case I test.
observable.subscribe(sink); | ||
}).to.throw('error!'); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test fails without the two other changes, so I made it as a separate commit to make it a bit easier for someone to confirm if they want
…rowing no longer trapped - Applies a fix from ReactiveX#3161 to prevent some sync errors from being trapped - Updates tests to not be so noisy when the flag is flipped - Tests that flipping the flag will console.warn and console.log appropriately
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is an attempt at fixing the rethrow issues in #2813. Some of them have been fixed by previous PRs, but other cases still remain. So far every one I've seen has been fixed by this PR.
I'll be totally honest though, why this works isn't 100% clear. The
syncErrorThrowable
stuff is pretty hairy. I tried to map out whatsyncErrorThrowable
is being used for, when its set correctly, when its not, etc and noticed a pattern that errors were going down the_trySubscribe
when there was athis.source
but that always expectedsyncErrorThrowable = true
otherwise the condition right after wouldn't do the rethrowing.Mostly what's not clear is why this only happens with Subject, timer, interval, etc combined with a nested Observable via merge/concat/etc. Seemed like it might be related to the fact that these override
_subscribe
but others do too and I couldn't find a pattern otherwise.The minimum reproduction here is, somewhat strangely, the inverse of what most often the problem seems to be. I tried to also find a minimum reproduction for what I think is the issue most often found (swallowing instead of rethrowing) I couldn't. The closest I could find was this:
I'm hoping that either this is good enough or perhaps this will help someone get a more clear understanding of the problem.
Fixes #3183, #2813