Removal of unsanctioned introduction of context

[3n-mb]

Description:
Observing functions (callbacks) are given to .subscribe(next, error, complete) by an outside code. But rxjs dictates a context of execution, instead of leaving it undefined. It sort of disrespects separation between callbacks that come from outside, and its own internals on the public API of the library.
As a result, there are issues #3229, #1981, #1949, #2140, that happen due to rxjs mangling with execution context of externally given callbacks. Library behaves in an unexpected way.

This change fixes an unsanctioned introduction of context to callback.
Now, it is possible, that this weird behaviour was introduced to help some other parts of the lib. Tests should show this. And may be those places should change instead of keeping users of the library puzzled (number of issues times minimum 1-2 hours, plus unreported cases).

Related issue (if exists): #3229, #1981, #1949, #2140

If this will be a no-fix, at least put comment near the place that is pointed in a stack overflow error, which people get. Put some indications for poor devs 😢

[rxjs-bot]

	Warnings
⚠️	commit message does not follows conventional change log (1)

	Messages
📖

(1) : RxJS uses conventional change log to generate changelog automatically. It seems some of commit messages are not following those, please check contributing guideline and update commit messages.

CJS: 1401.7KB, global: 750.2KB (gzipped: 120.6KB), min: 145.4KB (gzipped: 31.4KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage remained the same at 97.111% when pulling 5887c4f on 3n-mb:patch-1 into d7cfb42 on ReactiveX:master.

[benlesh]

Can you please add a test that would fail should this change?

[3n-mb]

@benlesh I don't think that a test for this tiny beautification of code failure is needed. I bet, that no-one would need to change this anyway. And if someone does, she will have reasons stronger than mere human error comprehension concern.
Initially, I thought that a reason for this code was something, but, judging from tests passing, original assignment was a way to satisfy typescript compiler (I was in similar places myself 😄 ).

[benlesh]

The point was to add a test that is broken before the fix, so we can see that the fix works. I can't merge this without a test.

[3n-mb]

@benlesh okey, then I need your help here.

• We may have a check that ensures that thrown error is TypeError. How to write this? There are no strict runtime types in JS (Java, Rust). So, how do you suggest this check be done? I never done it myself in JS.
• Where should file be placed? Or, into which file should a test case be added?
• What name should I give this test? What convention do you use here?
• Give me a link to some example test, to assimilate form, that is accepted by rxjs.

[benlesh]

Really the test should come first. Basically any code that is broken before the change, and some assertions around it. Just keep in mind that it can't run for a long time, because it'll slow down all of our builds.

[cartant]

Since commenting on the linked issue, I've seen this tweet from @trxcllnt:

TBF the next fn should be a non-arrow fn & do “this.unsubscribe()” instead. Combining observer + disposable was to help cut down on the number of intermediate disposables allocated in hot paths, but we clearly didn’t always follow our own advice.

The tweet suggests that using the subscriber as the context was a design decision and that it should not be replaced with undefined.

Note that this PR does not fix an error. Rather, its change effects a TypeError instead of a RangeError (see this explanation) if an unbound method is passed like this:

obs$.subscribe(
  observer.next,
  observer.error,
  observer.complete
);

Interestingly, this is also used as the context for some observable and operator subscribers - see the calling of the project function in map and of the result selector in forkJoin.

[3n-mb]

@cartant Wow!
But the change in this PR didn't break tests. 🤔
This leads to two options:

• there need to be a test ( @benlesh ) that will flag current change as incompatible;
• design, described in a tweet, has long been changed, and nobody noticed, cause no tests failed, of cause.

If current design stays, do

• add documentation, and
• place a comment(s) in a place(s) from which errors are coming in Stack overflow in Subscriber.js #3229, observer.next does not allow point free style #1981, fix(bufferTime): do not create buffer when unsubscribed #1949, Maximum Call Stack Exceeded When Passing oberver.next #2140.

[3n-mb]

@cartant can you point to a code that illuminates quoted part from tweet:

the next fn should be a non-arrow fn & do “this.unsubscribe()” instead. ...

My callback should be responsible to call unsubscribe?
<rant>Definitely, coding on tweeter with an artificial words limit isn't helping understanding.</rant>

[benlesh]

Yup. There's the breaking change. Although, IMO, Current behavior is also leaking implementation.

There are other proposals to fix this. A start event, and one that passes the subscription to the next handler as the second argument.

This all requires more thought. But I'd like to eliminate the implementation leaks.

[trxcllnt]

@cartant aye, good eye. The goal of context-rebinding in SafeSubscriber was to ensure anonymous Observers have same basic capabilities as you'd get by subclassing Subscriber, mostly access to the next, error, complete, and unsubscribe methods, and the isUnsubscribed flag. Later, this was extended to ensure Observable teardown functions are faithfully executed if an error is thrown while synchronously next'ing to a sink Observer. More context here and here.

Combining Observer and Disposable into a single class was indeed to cut down on intermediate disposables, as well as optimizing the Observer chain by storing references to sink Observers as instance properties rather than closure state. The decision to use the this context to unsubscribe isn't my favorite, but was made in the absence of better (more thought out, battle-tested etc.) alternatives that realized the same performance numbers. As @benlesh mentions, this is an area of active research.

To be clear, the challenge here was to ensure Observers could unsubscribe from potentially infinite synchronous sources (again, in the absence of something like cancelation tokens, Observer#start(), or being passed the subscription as a second argument):

Observable.range(0, Infinity).subscribe((function(x) {
  if (x > 100) { this.unsubscribe(); }
}));

A better way to do the above is to use take(100), but we didn't want to make it impossible to unsubscribe just because someone happens to be doing something differently than how I'd do it.

I know we do have this test that validates the context rebinding happens correctly on duck-typed anonymous Observer objects, but I can't seem to find any that validate the context is maintained for PartialObservers.

Also I seem to remember a conversation with @jayphelps a while back about this.unsubscribe() called in the manner above not actually working properly, which also may be why we don't have a test for it. I can't seem to find any issues or PRs about it, but that may be a thing we should look into fixing if it's still the case.

[trxcllnt]

Also to note, we bend over backwards to ensure the this context works like I described above -- it would be way easier if we didn't have to do any of that, so I'd be very receptive to any suggestions that solve all these problems without all the monkeying about with this contexts.

[3n-mb]

Looks like a rabbit hole goes deep.
As a pedestrian, who tripped over this pothole, can I ask you, guys, to

• put comment on let context: any = this;, like a flag near a pothole;
• comment in places that are referred in errors, seen by other pedestrians in Stack overflow in Subscriber.js #3229, observer.next does not allow point free style #1981, fix(bufferTime): do not create buffer when unsubscribed #1949, Maximum Call Stack Exceeded When Passing oberver.next #2140, so that folks are less surprised;
• add docs, so that members and owners of this very project are aware. And with more eyes on it, you may faster get a decent replacement, if it is possible, of cause, given constraints of js environment.

[benlesh]

Seems like we've worked this out, I'm going to close it.

[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.