fix: Ensure RxJS users don't create memory leaks

[benlesh]

There is a bit of excitement in the RxJS community about Svelte.

• It seems like the rest of Svelte "just works™" with RxJS!
• BUT The danger is that unwary users will figure out how smooth this API is and accidentally create nasty memory leaks if the returned RxJS Subscriptions are not handled. Fortunately the required change is small.

NOTE: I am not entirely sure how to test this change. The goal here is to make sure that whenever you would normally teardown your store subscriptions, it is also tearing down these RxJS-shaped subscriptions. This is most commonly something you want in a component scenario. Say you have a timer component in your app that you show and remove with an {#if} block, when the {#if} block hides the component, you'd want to tear down the underlying Observable that is "ticking".

Also worth noting: I totally understand if this sort of change isn't palatable. It's mostly my concern that RxJS users excited about Svelte will make mistakes that are really nasty to debug. If two lines of code solves that, then I had to try. 😄

Related #2549

[Rich-Harris]

this is great, thank you. We can add a test tomorrow — for now, just had that small question about the API

[benlesh]

Cool. I responded to your comment and committed your suggestion.

[steve-taylor]

I don't think this is the right way to provide this kind of support. The Observable subscription contract is just one of many. How will we support other contracts? This change doesn't take us in a sustainable direction.

With the current contract, in which the subscription is a function that unsubscribes, we can build adapters around other contracts.

I would like to propose the following generic adapter:

function asReadable(subscribe, unsubscribe) {
    return {
        subscribe(run) {
            const subscription = subscribe(run)
            return () => void unsubscribe(run, subscription)
        }
    }
}

This function returns an object with the same contract as a readable store.

We can build on this to create an adapter for Observables:

function fromObservable(observable) {
    return asReadable(
        (run) => observable.subscribe(run),
        (run, subscription) => void subscription.unsubscribe()
    )
}

Usage example:

<script>
    import { fromObservable } from 'svelte/store'; // or from somewhere else
    import { ajax } from 'rxjs/ajax';
    import { pluck, startWith } from 'rxjs/operators';

    const users$ = fromObservable(ajax('https://api.github.com/users?per_page=5').pipe(
        pluck('response'),
        startWith([])
    ));
</script>

{#each $users$ as user}
    <div>
        {user.login}
    </div>
{/each}

If we later want to use EventEmitters:

function fromEventEmitter(eventEmitter, eventType, options) {
    return asReadable(
        (run) => void eventEmitter.on(eventType, run, options),
        (run) => void eventEmitter.off(eventType, run, options)
    )
}

And so on. Svelte's internal subscription code will remain clean and doesn't have to figure out how to subscribe and unsubscribe.

[Rich-Harris]

The existing store is the generic adaptor, no? It looks like this:

import { readable } from 'svelte/store';

function fromEventEmitter(eventEmitter, eventType, options) {
  return readable(null, set => {
    eventEmitter.on(eventType, set, options);
    return () => eventEmitter.off(eventType, set, options);
  };
}

This PR is just about making even that part unnecessary in the case of RxJS TC39 Observables.

[steve-taylor]

The existing store is the generic adaptor, no?

Thanks for pointing that out. readable as an adapter is interesting from an RxJS point of view, because its observables are (unfortunately) cold. Each subscriber to an observable recomputes it, so you end up accidentally doing things like peppering endpoints because you have multiple subscribers. The way readable only calls start when the subscriber count goes from 0 to 1 and only calls stop when it goes from 1 to 0 makes it ideal, even necessary, for supporting auto-subscribing to RxJS observables.

import { readable } from 'svelte/store';

function fromObservable(observable) {
  return readable(null, set => {
    const subscription = observable.subscribe(set);
    return () => void subscription.unsubscribe();
  });
}

I'm just speculating, of course, that autosubscribing to the same thing in multiple places in the markup will cause multiple subscriptions to the store. Is that the case or not?

This PR is just about making even that part unnecessary in the case of RxJS TC39 Observables.

A couple of issues I have with this:

• We're now checking all the time whether we're unsubscribing from a store or an Observable; and
• This is going in an unsustainable direction, or it's at least locking us into not natively supporting other types of contracts.

[Rich-Harris]

I'm just speculating, of course, that autosubscribing to the same thing in multiple places in the markup will cause multiple subscriptions to the store. Is that the case or not?

There'll be as many autosubscriptions as there are components that subscribe to a particular store, yeah. Each component will only have one subscription.

We're now checking all the time whether we're unsubscribing from a store or an Observable

The overhead is negligible though, and at any rate unsubscriptions don't typically live in a hot code path IME

This is going in an unsustainable direction, or it's at least locking us into not natively supporting other types of contracts

I don't think we want to support other types of contracts. RxJS is a bit of a special case because it's already a de facto standard in large parts of the JS ecosystem, and because the contract is the same (certainly for current intents and purposes) as the proposal currently working its way through TC39. The other special consideration is that RxJS very nearly works natively with Svelte, and people could easily fall into the trap of using them without realising that they're risking memory leaks etc. So I think that for the sake of a few more bytes, it's worth making this small exception

[steve-taylor]

Each component will only have one subscription.

Excellent. At least cold observables won't be an issue for non-exported observables.

There'll be as many autosubscriptions as there are components that subscribe to a particular store, yeah.

That's going to create an issue with cold observables that just isn't the case with stores. Using an adapter based on readable would prevent this from happening as it insulates against any effects of multiple subscribers.

[johnlindquist]

as many autosubscriptions as there are components that subscribe

RxJS supports "sharing", counting, tracking, removing, adding subscriptions through the subscriber API and related operators.

https://rxjs.dev/api/index/class/Subscription

The burden is on the User to set up the Stream with the appropriate operators (which use Subjects under the hood) to support those features:

https://rxjs.dev/api/operators/share

Meaning, Svelte's only job would be to subscribe/unsubscribe. The User is in charge of handling multiple subscriber behavior.

[benlesh]

The real problem is that inevitably users of two very popular libraries are going to figure out that this "just works" with or without this patch, and not understand they are creating memory leaks. Such leaks can be very hard to debug.

This PR is as much about mitigating risk as it is about the supporting Observable.

[steve-taylor]

The burden is on the User to set up the Stream with the appropriate operators (which use Subjects under the hood) to support those features:

It’s not obvious that exported observables have to be shared, whereas private ones don’t. It’s also not obvious whether or not the observable should replay its last event to subscribers in order to play nice with Svelte. In fact, it has to do both of these things, which means share() isn’t good enough. It would have to be something like

• shareReplay(1),
• publishReplay(1), refCount(), or
• multicast(() => new ReplaySubject(1)), refCount()

The first option may not ever unsubscribe. The second option may emit a stale value when the sibscriber count goes from 1 to 0 to 1 again. (Is that possible in Svelte?) The third option seems to be the one that best emulates stores, although I haven’t tried it.

Are you starting to see that there’s more to the contract? By allowing users to use something other than stores, we’re giving them an entirely new set of problems.

The real problem is that inevitably users of two very popular libraries are going to figure out that this "just works" with or without this patch

It half works and only by accident. Stores and observables happen to have the same subscribe function name.

Without this patch, wouldn't we get an error due to Svelte attempting to call a subscription object, rather than a memory leak?

[tomblachut]

Adapter could be provided in another PR as a refined option/optimisation technique

[Rich-Harris]

I definitely think the arguments in favour outweigh the arguments against. Added a test, and just as well — Ben, we both missed that unsub = () => unsub.unsubscribe() breaks because it redefines unsub! Will cut a release shortly.