fix(subscribe): prevent swallowing of errors when stopped #3469

cartant · 2018-03-24T07:47:05Z

Description:

If _trySubscribe catches an error and the sink - or one of its destinations - is stopped and the sink's error method is called, the error is swallowed - as it cannot be reported via the observer's callback. This commit adds an internal reportError method to Subscriber. The method defers to the appropriate error reporting mechanism if a stopped subscriber is found.

For an example scenario of an error being swallowed due to a bug and a closed subscriber (that has not previously been notified of any error) see #3444.

Related issue (if exists): #3461

benlesh · 2018-03-24T18:49:28Z

src/internal/Subscriber.ts

+   * subscribers and uses the appropriate unhandled mechanism to report the
+   * error if a stopped subscriber is found.
+   */
+  reportError(err?: any): void {


This should be private or protected. And ideally named with _ prefix to discourage external use.

Done. It's now private and has a leading _. However, its being private means that a cast to any is required in _trySubscribe and in the tests.

benlesh · 2018-03-24T18:53:59Z

src/internal/Subscriber.ts

+  reportError(err?: any): void {
+    let observer: PartialObserver<T> = this;
+    while (observer) {
+      if (observer instanceof Subscriber) {


I'm not sure about this check. When would this not be the Subscriber? If for some reason it gets wrapped in a SafeSubscriber?

this will always be a Subscriber. It's just that observer is the iteration variable. PartialObserver<T> is the type of the destination property - which is assigned to observer during the iteration.

The iteration starts with this so that its isStopped property is checked. And it's all done in the one loop so that the hostReportError call, etc. is just in the one place.

benlesh · 2018-03-24T18:54:46Z

src/internal/Subscriber.ts

+    while (observer) {
+      if (observer instanceof Subscriber) {
+        const { destination } = observer;
+        if (observer.isStopped) {


It seems like this could pass if someone just calls error twice in a row on Subscriber, no?

~~The existing mechanism for guarding against re-enterant calls to error should not be compromised. _reportError is called only from within the catch inside _trySubscribe.~~

Even if a caller subverts the private qualification (added per request above) and calls _reportError instead of error, a Subscriber in the chain will still become stopped and further calls to either _reportError or error will not see the observer's error callback invoked.

I'm wrong. The implementation needs to set isStopped to true for ~~the subscriber at the root of the~~ each of the subscribers in the destination chain. I'll do that and add another test.

I've updated the implementation so that error is called on the chain's root subscriber. This will see calls to error flow through the chain of subscribers and each will be marked as stopped.

Even if a particular subscriber is used as a destination for another subscriber that itself is not part of the chain, the observer's error callback for that particular subscriber will be invoked only once. So the existing mechanism for guarding against re-enterant calls to error should not be compromised.

_reportError is called only from within the catch inside _trySubscribe, but even if a caller subverts the private qualification (added per request above) and calls _reportError instead of error, all of the subscribers in the chain will still become stopped and further calls to either _reportError or error will not see the observer's error callback invoked.

cartant · 2018-03-24T21:45:35Z

@benlesh I'm finished making changes that relate to the review's comments.

cartant · 2018-03-25T04:55:37Z

It occurred to me that reportError could be made less visible by using a Symbol and the method would then be callable without an as any assertion in _trySubscribe and in the tests.

I've updated the PR to do this.

If _trySubscribe catches an error and the sink - or one of its destinations - is stopped and the sink's error method is called, the error is swallowed - as it cannot be reported via the observer's callback. This commit adds an internal reportError method to Subscriber. The method defers to the appropriate error reporting mechanism if a stopped subscriber is found. Closes ReactiveX#3461

benlesh · 2018-03-27T04:36:12Z

Actually, the instanceof Subscriber check is going to be problematic. I just spent a while tracking down a nasty bug in node related to an instanceof Subscriber check (See #3477 and #3476). So I'm not quite ready to merge this one. I'll need to think about it what the implications are a little more carefully.

cartant · 2018-03-27T04:43:25Z

Yes, I see what you mean. The implementation in this PR would have to use the isTrustedSubscriber function that you added in #3476.

cartant · 2018-03-27T05:56:10Z

My take on it is that instanceof Subscriber will fail with foreign Subscriber instances and that will see error called on the root subscriber - even if the foreign Subscriber (or a subscriber further along the chain) is stopped. And that would see the error swallowed and not reported.

Technically, using isTrustedSubscriber as a custom type guard is overkill - the types are enforced in the constructor, so it's only necessary to discern whether it has a destination property or is just a PartialObserver - but using isTrustedSubscriber is probably better than introducing a seemingly redundant custom type guard. At least that way, it's apparent that instanceof Subscriber is dangerous.

benlesh

I'm not sure this is the right fix, now that I'm looking at it. See my other comment.

benlesh · 2018-03-27T16:10:21Z

src/internal/Subscriber.ts

+      } else {
+        this.error(err);
+        return;
+      }


it looks like this loop will continue to hit hostReportError over and over for every level at which a subscriber isStopped. So if it has this.destination.destination.destination and all are stopped, the same error will be reported 3 or 4 times.

Also, I'm trying to understand the bug this is solving. The isStopped flag is meant to guard against code calling subscriber.complete(); subscriber.error(new Error()); or calling error twice, etc.

It seems like the real fix might be to figure out why the Subscriber is prematurely stopped in the first place. Sorry, I should have caught that in an earlier review. Perhaps I missed something in one of your earlier comments?

Sorry, I should have caught that in an earlier review.

No worries. With PRs like this - where there isn't a prior, agreed-upon solution that was discussed in an issue - I'm aware that it could all be for nought. It could be the wrong approach or could just be plain wrong, etc. I probably should have mentioned that at the outset, as it's never fun having to tell someone you don't want to merge a PR.

Anyway, this PR is far from urgent.

Also, I'm trying to understand the bug this is solving.

The swallowing of the error is problem that it solves. If an error is thrown after a subscriber becomes stopped - as in #3444 - _trySubscribe attempts to report it via which error is pointless. The motivation for the PR was the appearance of the bug in #3444 in 5.5.6 and its disappearance in the beta. I thought that if a change to the sync error handling was made in 5.5.6 - to better report errors - it would be preferable if the beta didn't go back to swallowing them.

I guess swallowed errors are a pet peeve of mine.

It seems like the real fix might be to figure out why the Subscriber is prematurely stopped in the first place.

In #3444, the reason the subscriber became stopped was due to notifyComplete being called by the inner subscriber. The notifyComplete implementation called super.complete() and stopped the subscriber. However, with a synchronous notifier there was more code that ran after that and there was a bug in that code.

It might be possible to re-arrange repeatWhen so that becoming stopped is guaranteed to be the very last thing that occurs, but situations in which errors are swallowed could re-arise if someone were to write a wierd, user-land repeat-ish operator.

I imagine that the types of errors that this PR would expose - that would otherwise be swallowed - are very rare. I doubt there are too many operators like repeatWhen that have to fudge with the forwarding of complete notifications.

it looks like this loop will continue to hit hostReportError over and over for every level at which a subscriber isStopped

I don't believe that's the case; the return will exit the function and, as it doesn't re-throw, there's no error to be caught by any outer subscribe calls. Basically, there are three ways out of the loop:

the old, sync error behaviour of re-throwing the error;

calling hostReportError and returning;

calling error and returning.

Of those three, I'm quite sure that the last two are correct. However, I'm less certain about the first: the throw. I think it's okay, but I will need to give it some more thought.

benlesh · 2018-03-30T19:09:16Z

I think the core team will have to have a discussion about what Subscribers should do when error is called after the subscriber is stopped (due to another error call, completion, or unsubscription). Historically, we've always ignored the second call. There are a variety of reasons for this.

Until then I think we can either block or close this PR, as I don't think I can land it in v6 and it would most definitely be a breaking change after that.

cartant · 2018-03-30T23:26:48Z

I think the core team will have to have a discussion ...

Yeah, in light of #3487 - which is almost the same, but with a completed/closed subject instead of a completed/stopped subscriber - it would make a lot of sense to close this PR and institute an across-the-board policy for the reporting of errors if an observer's error method cannot be called.

If the deprecated sync error handling is going to be removed in v7, calling hostReportError in all situations in which an observer's error method cannot be called would make a lot of sense. And it would be simple. Unlike this PR.

benlesh · 2018-04-02T17:22:47Z

#3487 was a little different, as it has to do with the specific implementation of bindCallback, but reporting errors to hostReportErrors in situations where error couldn't be called would definitely help there, yes.

cartant · 2018-04-02T21:40:13Z

I'm really only talking about errors caught with a try/catch and then reported by calling error on an observer.

The suggestion is that a specific function could be called in those situations and said function could decide whether or not to report via the observer or the host.

Something like this:

export function reportError(err: any, observer: ErrorObserver<any>): void {
  if (canReportError(observer)) {
    observer.error(err);
  } else {
    hostReportError(err);
  }
}
function canReportError(observer: ErrorObserver<any>): boolean {
  const { closed, destination, isStopped } = observer as any;
  if (closed || isStopped) {
    return false;
  } else if (destination) {
    return canReportError(destination);
  }
  return true;
}

It'd need better type checking, but that's the basic concept.

Changing the behaviour of error itself would be simpler still, but I'd have to go and re-read the observable contract and the TC39 proposal to see if that violates any guarantees.

slashhuang · 2018-04-04T05:40:22Z

have the same thought like @cartant
observer error is swallowed and cannot be caught.

{
   next: () => {
     // throw some error here
  },
  error: (err) => {
   // won't trigger,  but unsubscribe parent observables
 },
}

benlesh · 2018-05-04T18:37:35Z

Okay, going to close this one for now, pending further discussion.

lock · 2018-06-05T19:35:25Z

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.

benlesh requested changes Mar 24, 2018

View reviewed changes

cartant force-pushed the issue-3461-2 branch from ac060db to f9c5a98 Compare March 24, 2018 21:38

cartant added 4 commits March 27, 2018 14:22

test(subscribe): add failing should-report test

8373248

chore(review): make requested changes

734fe54

refactor(reportError): use a symbol

47891be

cartant force-pushed the issue-3461-2 branch from 1d926b5 to 47891be Compare March 27, 2018 04:23

cartant added 2 commits March 27, 2018 15:19

refactor(subscriber): make isTrusted a type guard

fb193b3

fix(reportError): use isTrustedSubscriber

dc760de

cartant changed the title ~~fix(subscribe): add internal reportError method~~ fix(subscribe): prevent swallowing of errors when stopped Mar 27, 2018

benlesh requested changes Mar 27, 2018

View reviewed changes

benlesh added blocked PR: Requires Further Review and removed blocked labels Mar 30, 2018

cartant mentioned this pull request Apr 2, 2018

Errors swallowed in stopped subscribers #3461

Closed

cartant mentioned this pull request Apr 26, 2018

subscribe() insists on throwing already-handled errors #3560

Closed

benlesh closed this May 4, 2018

lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018

cartant deleted the issue-3461-2 branch September 24, 2020 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(subscribe): prevent swallowing of errors when stopped #3469

fix(subscribe): prevent swallowing of errors when stopped #3469

cartant commented Mar 24, 2018 •

edited

Loading

benlesh Mar 24, 2018

cartant Mar 24, 2018

benlesh Mar 24, 2018

cartant Mar 24, 2018

benlesh Mar 24, 2018

cartant Mar 24, 2018 •

edited

Loading

cartant Mar 24, 2018 •

edited

Loading

cartant Mar 24, 2018 •

edited

Loading

cartant commented Mar 24, 2018

cartant commented Mar 25, 2018

benlesh commented Mar 27, 2018

cartant commented Mar 27, 2018

cartant commented Mar 27, 2018 •

edited

Loading

benlesh left a comment

benlesh Mar 27, 2018

cartant Mar 27, 2018 •

edited

Loading

benlesh commented Mar 30, 2018 •

edited

Loading

cartant commented Mar 30, 2018 •

edited

Loading

benlesh commented Apr 2, 2018

cartant commented Apr 2, 2018 •

edited

Loading

slashhuang commented Apr 4, 2018 •

edited by benlesh

Loading

benlesh commented May 4, 2018

lock bot commented Jun 5, 2018

fix(subscribe): prevent swallowing of errors when stopped #3469

fix(subscribe): prevent swallowing of errors when stopped #3469

Conversation

cartant commented Mar 24, 2018 • edited Loading

benlesh Mar 24, 2018

Choose a reason for hiding this comment

cartant Mar 24, 2018

Choose a reason for hiding this comment

benlesh Mar 24, 2018

Choose a reason for hiding this comment

cartant Mar 24, 2018

Choose a reason for hiding this comment

benlesh Mar 24, 2018

Choose a reason for hiding this comment

cartant Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

cartant Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

cartant Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

cartant commented Mar 24, 2018

cartant commented Mar 25, 2018

benlesh commented Mar 27, 2018

cartant commented Mar 27, 2018

cartant commented Mar 27, 2018 • edited Loading

benlesh left a comment

Choose a reason for hiding this comment

benlesh Mar 27, 2018

Choose a reason for hiding this comment

cartant Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

benlesh commented Mar 30, 2018 • edited Loading

cartant commented Mar 30, 2018 • edited Loading

benlesh commented Apr 2, 2018

cartant commented Apr 2, 2018 • edited Loading

slashhuang commented Apr 4, 2018 • edited by benlesh Loading

benlesh commented May 4, 2018

lock bot commented Jun 5, 2018

cartant commented Mar 24, 2018 •

edited

Loading

cartant Mar 24, 2018 •

edited

Loading

cartant Mar 24, 2018 •

edited

Loading

cartant Mar 24, 2018 •

edited

Loading

cartant commented Mar 27, 2018 •

edited

Loading

cartant Mar 27, 2018 •

edited

Loading

benlesh commented Mar 30, 2018 •

edited

Loading

cartant commented Mar 30, 2018 •

edited

Loading

cartant commented Apr 2, 2018 •

edited

Loading

slashhuang commented Apr 4, 2018 •

edited by benlesh

Loading