Errors thrown in Subscriber's next handler should call unsubscribe logic

[benlesh]

It was supposed to be this way in Rx 4.0.6, according to @mattpodwysocki ... but when I tried it it didn't work out that way.

[benlesh]

Digging into this, I'm not sure this can be implemented reliably. This is because any synchronous call to next will happen before the subscriber function returns the teardown/unsubscribe logic:

const source = new Observable(observer => {
  // start some expensive async thing
  var id = asyncThing((x => observer.next(x));

  // synchronously emit a value (that will throw ultimately)
  observer.next('wee');

  // return teardown logic for expensive async thing
  return () => {
    cancelAsyncThing(id);
  };
});

source.subscribe((x) => {
  if (x === 'wee') {
    throw new Error('stop having fun!');
  }
});

In the above, cancelAsyncThing will never be called. It doesn't matter if we wrap the next handler in a try/catch and try to call unsubscribe, because the Observable's setup logic hasn't even completed yet to even return the unsubscribe logic.

There's basically no way to get this to work consistently unless all Observable implementations had a try/finally block.

I'm not sure what we should do for expected behavior here. But I think it should be consistent so as not to surprise developers.

cc/ @zenparsing @jhusain @headinthebox

[benlesh]

I suppose we could squelch all next, error and complete calls after the error is thrown, to allow the subscriber logic to complete and return the teardown, then unsubscribe.

[trxcllnt]

@Blesh resolved in #1191. It also resolves some of @mattpodwysocki and @zenparsing's concerns about the difference between using callbacks and a duck-typed Observer object.

[benlesh]

@trxcllnt did you happen to test perf before and after? I suspect that adding several conditionals and a few function calls to each emission is going to impact performance.

[trxcllnt]

@Blesh I ran node perf/micro mergemap-observable and saw no regressions. I haven't run the full micro/macro suite though.

[benlesh]

This is tricky, because the ideal behavior is to halt execution of the subscription logic and teardown only what's been subscribed to at that point.

In effect, it would almost be better to have teardown logic added to subscriber via subscriber.add than it is to have to get to the end and return the teardown logic.

[benlesh]

This was resolved by #1191.

[lock]

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.