Stack overflow in Subscriber.js

[3n-mb]

RxJS version: 5.5.6 (used in node)

Code to reproduce:

// we want to use a subject-backed shared observable
const subj = new rxjs.Subject();
const obs$ = subj.asObservable().share();
// want to wrap subscription into a function (library's external api)
const watch = (observer) => {
  const sub = obs$.subscribe(observer.next, observer.error, observer.complete);
  return () => sub.unsubscribe();
};
// using subscription function
const prom = (new rxjs.Observable(observer => watch(observer)))
.take(1).toPromise();

subj.next(5);  // <-- this throws up
prom.then(n => console.log(n));

Expected behavior: complete without an error, and print 5

Actual behavior: line subj.next(5); overflows the stack, throwing the following error:

RangeError: Maximum call stack size exceeded
  at SafeSubscriber.Subscriber.next (.../node_modules/rxjs/Subscriber.js:88:42)
  at SafeSubscriber.Subscriber.next (.../node_modules/rxjs/Subscriber.js:90:18)
  at SafeSubscriber.Subscriber.next (.../node_modules/rxjs/Subscriber.js:90:18)
  at SafeSubscriber.Subscriber.next (.../node_modules/rxjs/Subscriber.js:90:18)
...

Additional information:

• If code is changed to not use subscription wrapping function, then all goes well:

// we want to use a subject-backed shared observable
const subj = new rxjs.Subject();
const obs$ = subj.asObservable().share();
// using subscription function
const prom = obs$
.take(1).toPromise();

subj.next(5);  // <-- goes fine
prom.then(n => console.log(n));

• It is not obvious that function watch is creating a function call loop. Yet, it seems that a combination of watch and a subscription via creating a new observer new Observer(...) establishes a loop that throws up.

Let me know if code for watch function violates rxjs' api. And if not, this is a bug.

[kwonoj]

🤔 this looks bit odd, it seems like we're keep trying to create safeSubscriber internally.

as an immediate workaround, you can pass observer directly

const watch = (observer) => {
  //const sub = obs$.subscribe(observer.next, observer.error, observer.complete);
  const sub = obs$.subscribe(observer);
  return () => sub.unsubscribe();
};

and stack won't blow up.

I may need dig what's happening internally still.

[3n-mb]

@kwonoj
Code runs if watch looks as follows:

const watch = (observer) => {
  const sub = obs$.subscribe(observer);
  return () => sub.unsubscribe();
};

i.e. if observer is not split into its respective functions.

Doubly odd :)

[3n-mb]

Looking at Subscriber.js, is SafeSubscriber messes things up?
Have no further ideas.

[cartant]

The problem is that unbound methods are passed to subscribe. If bind is used, the problem is solved:

const watch = (observer) => {
  const sub = obs$.subscribe(
      observer.next.bind(observer),
      observer.error.bind(observer),
      observer.complete.bind(observer));
  return () => sub.unsubscribe();
};

observer will be an instance of Subscriber-derived class and the implementation of its next method will use this.

[3n-mb]

Wow!
Very true that unbound functions are passed. Use of my code has bound functions on an observable, and this time I used Observer that came from rxjs in new Observable(observer => ...).

Why wasn't this undefined, as it usually happens with such cases?

Does it worth to change code, so that instead of stack overflow error, there is something else, due to this being undefined?

[cartant]

@3n-mb The problem is that passing an unbound observer.next creates a situation in which there is a recursive call.

When the unbound observer.next method is passed to subscribe, it ends up in a SafeSubscriber - as subscribe calls toSubscriber which creates a Subscriber. And that Subscriber contains a SafeSubscriber.

If you look at the implementation of SafeSubscriber, you'll see that when it calls the this._next function, it uses this._context as the context for the call:

fn.call(this._context, value);

this._context is initialized as the this for the SafeSubscriber instance.

That means the call to fn sees Subscriber.next called - which, in turn, calls Subscriber._next. The destination that's used in Subscriber._next will come from the SafeSubscriber context and the destination.next will be the unbound observer.next that was passed to subscribe. This sets up a recursive call that sees the stack overflow error effected.

In short, there is nothing to be fixed in RxJS.

[3n-mb]

I see no reason to do

let context: any = undefined; // instead of this

on this line of Subscriber.ts.

Unless there is something else outside of SafeSubscriber relies on calling next/error/complete functions with particular non-undefined context. But why should such expectation exist?

Can you try run tests with this change? I can't PR on a tag 5.5.6.

[cartant]

@3n-mb Maybe. I know a little about the subscribe/subscription mechanisms in RxJS - I've poked around in those parts when building tools - but I sure don't know everything. Others will know more than me.

Whether or not undefined is appropriate, I don't know. In relation to this issue, I've only looked at the handling of the next notification. You'd also need to be certain that the context is irrelevant for the error and complete notifications.

Seeing no reason not to change something is not quite the same as having a compelling reason for something to be changed. What's the upside of the change? A slightly better error message? I'm not sure how often the error would occur anyway, as it's usually the case that the observer itself is passed. Although, it seems it has been raised before: a quick search of the repo found an identical issue.

Anyway, if you are keen to put in the effort to verify that the change is appropriate and has no ill effects and to convince the maintainers of such, you should make the PR against master. The maintainers can cherry pick the commit into stable - which is the branch for 5.5.6.

[3n-mb]

@cartant yes, there are now four issues! I mentioned all of them in a PR.

Here is a view on this problem:

1. .subscribe(...) is exposed as a public api,
2. it accepts external callback,
3. it changes execution context of a callback that comes from an outside.

I have a huge problem with (3). And judging by the number of issues, I am not alone. Cause library behaves unexpectedly, unless, of cause, documentation says that given callbacks will be executed in some context (RTFM excuse 😄 ).

[benlesh]

The original example in here doesn't make any sense. I mean, I can follow it, but I have no idea what the intention is, and it makes it hard to see the problem.

What is it that you want to do, @3n-mb, in the simplest possible example?

[3n-mb]

@benlesh

... I have no idea what the intention is ...
What is it that you want to do, @3n-mb, in the simplest possible example?

The short reason for this code is in the comment:

// want to wrap subscription into a function (library's external api)
const watch = (observer) => { ... }

Let me unpack why library's external api needs watch function. Buckle up.

In 3NWeb effort we make an electron app that has a core (electron's main process) and other apps on top (articulated as electron's renderer processes in beautiful windows).
From a perspective of an app, core provides some capabilities (objects). These objects on renderer side are proxies to actual objects in core. Signaling goes over IPC between two processes.
Naturally, there is an API boundary between core and renderer. This boundary should allow things that happen to objects. With object, there are methods (call and wait interactions), and there are events, excreted by objects. What is the best way to deal with events? Observable's approach, of cause.
But, Observables ain't part of standard JS, and neither side should be restricted in implementations. Yet, we want to capture Observables' wisdom, and allow simpler integration when any side decides to use actual Rx library. So, solution is to have a watch function sticking on appropriate RPC'ed objects.

When I was making a test for RPC functionality, I created a mock, implemented with rxjs (I love this lib, thak you, guys). And I got an eye-popping, totally unexpected error about stack overflow. Something like TypeError about undefined would've made sense to me (and other folks in mentioned same issues).
So, I've cleaned out parts of the code, unrelated to the issue, and present it here.
I appreciate a thought: "why to wrap subscribe into something that walks and quacks like subscribe?". Indeed, sample code looks funky out of context: test mocks, for a library with an Observables-friendly interface style.

[benlesh]

Anyhow, the problem is that you're losing the context of the observer's functions when you pass them in as standalone functions. The context is actually assigned to the SafeSubscriber that "owns" those handlers and the subscription. This was actually done by design and we can't change it, at least right now.

Since the problem is solved by binding your observer's handlers to the original observer (as suggested above), I'm going to close this issue.

[3n-mb]

@benlesh @cartant This bit again!
Its a feedback.
I am writing correct JS code. And library is sticking fingers into my code by adjusting execution context. What is emoji for pain?