Observable from webpack library doesn't unsubscribe

[samal-rasmussen]

Bug Report

Current Behavior
The source observable doesn't unsubscribe when unsubscribing an observable that came from a library that returns rxjs observables.

The libray was bundled with Webpack.

Reproduction
https://github.com/samal84/rxjs-import-from-library-test

Environment

• Runtime: node 10.16.3, Chrome 77, Windows 10
• RxJS version: 6.5.3
• Webpack: 4.41.0

[BioPhoton]

Hi @samal84.

I run your repository to reproduce it. It's truly not working. I also changed you test observable to use Interval to even better demonstrate the problem.
PRs are also sent to your repo with the changes.

In your application part, I copied the same code that you compile to compare it. This works as expected.

So the problem lies in your compilation process and can IMHO not be fixed in the library.
I suggest looking for how rxjs is compiled and what the difference is.

If @kwonoj or @cartant agrees, I would suggest closing this one.

[kwonoj]

In example first rxjs is bundled in lib via webpack, and then another rxjs is bundled in application. This creates 2 sets of rxjs in application which doesn't passes instance check. Application should only have 1 set of observable.

[samal-rasmussen]

Thanks BioPhoton for the pull requests. I merged them.

I also added a passing test case without pipe or switchMap:

test$().pipe(
	take(1),
)
.subscribe(getObserver('compiled direct no pipe and switchMap'));

@kwonoj Does this mean that returning rxjs observable instances from libraries is not supported?

[kwonoj]

rxjs observable instances from libraries is not supported

no, library definitely can return observable as it needs, but in your application example you have 2 sets of rx (more than 1). That's problem need to be fixed.

[samal-rasmussen]

rxjs observable instances from libraries is not supported

no, library definitely can return observable as it needs, but in your application example you have 2 sets of rx (more than 1). That's problem need to be fixed.

The library in this case is using rxjs internally and then returning rxjs observables to callers. So it needs rxjs to be bundled along with it, right? Or else it cannot use rxjs internally.

The application is also using rxjs itself, so it also needs rxjs bundled with it for the same reason.

So this means that the lib and application get bundled separately, and get rxjs bundled into them separately.

How would we have only one rxjs when using rxjs inside both the lib and the app?

[samal-rasmussen]

Or I could try having the library bundle the whole of rxjs into it using npm bundledDependencies and have the applications that use the library import rxjs from the library:

https://docs.npmjs.com/files/package.json#bundleddependencies

I don't really want to even try this, as I don't want the applications to be dependent on the rxjs version that happens to be bundled in the library, and it seems like it would make the library bundle super heavy.

[cartant]

I think this could be a bug. I'll have a closer look at this tomorrow. Specifically, I think it might have something to do with this: https://github.com/ReactiveX/rxjs/blob/master/src/internal/symbol/rxSubscriber.ts#L4

[kwonoj]

@cartant it is not a bug. we do use instanceof checks as well after we remove symbol check #4182 (comment) which also does not gaurantee behavior acorss multiple instance.

from beginning of v5 we never supported multiple instances.

[cartant]

Nah. This is a bug for sure. I agree that the previous changes that related to different copies of RxJS were necessary because previous implementations were trying to be clever regarding trusted subscribers, but this problem is a little different. It should be possible to use observables from other, 'foreign' packages - including other versions or locations of RxJS. Basically, this problem is not cause by attempting to re-use a trusted Subscriber instance. It's caused by wrapping one in a SafeSubscriber.

I've added a failing test here:

https://github.com/cartant/rxjs/blob/4a347ee26feed258afb7fae5a8dd76aeba2e8385/spec/util/subscribeToObservable-spec.ts#L93-L108

And I'm thinking about the best way to solve it. The problem was introduced with a change to subscribeToResult that allowed it be be passed a specifically created subscription. And that subscription is added to the observable's destination - for teardown upon unsubscription. However, in the case of a 'foreign' observable, that subscription will be wrapped in a SafeSubscriber and because the wrapping SafeSubscriber is not added to the destination, it's not torn down upon explicit unsubscription.

Changing this

rxjs/src/internal/operators/switchMap.ts

Lines 131 to 140 in 865b7d3

	private _innerSub(result: ObservableInput<R>, value: T, index: number) {
	const innerSubscription = this.innerSubscription;
	if (innerSubscription) {
	innerSubscription.unsubscribe();
	}
	const innerSubscriber = new InnerSubscriber(this, undefined, undefined);
	const destination = this.destination as Subscription;
	destination.add(innerSubscriber);
	this.innerSubscription = subscribeToResult(this, result, value, index, innerSubscriber);
	}

To something like this fixes it:

  private _innerSub(result: ObservableInput<R>, value: T, index: number) {
    const innerSubscription = this.innerSubscription;
    if (innerSubscription) {
      innerSubscription.unsubscribe();
    }
    const innerSubscriber = new InnerSubscriber(this, undefined, undefined);
    const destination = this.destination as Subscription;
    destination.add(innerSubscriber);
    this.innerSubscription = subscribeToResult(this, result, value, index, innerSubscriber);
    if (this.innerSubscription !== innerSubscriber) {
      destination.add(this.innerSubscription);
    }
  }

but I'm not sure whether or not that's the best solution. And that mechanism is used in a number of operators.

Still thinking about it. But this is a bug.

[kwonoj]

I still disagree: it was known, expected behavior to not support multiple instance of observable in single context, reasonnwhy there is plugin like webpack-rxjs-externals exists. What we gaurantee is observable will work in an application when it have single instance only. For other cases, though we could say it's improvement request it was never supported.

[cartant]

The problem is that that cannot be easily enforced with Node. My recollection is that the fixes that were made were made to avoid problems when there are multiple RxJS packages in node_modules.

In any case, this is an interop problem, as the subscription returned by a subscribe call to an interop observable is ignored! It just so happens that RxJS packages in different locations are considered to be interop these days.

Specifically, with the changes made in these PRs, returning interop observables in a flattening operators will leak like crazy unless this bug is fixed:

• fix(mergeAll): add source subscription to composite before actually subscribing #2479
• Fix teardown issues with synchronous inner-observables #4037