Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add test from #5932 and plug leak in share #6029

Merged
merged 3 commits into from
Feb 19, 2021

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Feb 16, 2021

Description:

This PR brings in the test from #5932 and fixes a leak that it exposed in the share implementation.

Maybe we should add the test to the specs for share, too, but this is all I have time for, ATM.

Related PR: #5932

@benlesh
Copy link
Member

benlesh commented Feb 19, 2021

@cartant I've added another commit that more appropriately fixes the issue.

The problem was we were accidentally returning a teardown that was basically adding a closure over a subscriber to itself.

Effectively:

new Observable(subscriber => {
  return () => {
    subscriber.unsubscribe();
  };
});

It's not super obvious when you look at it, but that's what it's doing. const castSubscription = subject.subscribe(subscriber); was the confusing bit I think... because technically, castSubscription === subscriber. :)

Calling subscribe with our own Subscriber will return the same instance of Subscriber.

Resolves an issue where we were inadvertantly returning a teardown that was effectively adding a closure to a subscriber to itself.

Basically it was doing something like this:

```ts
new Observable(subscriber => {
    return () => {
        subscriber.unsubscribe();
    };
});
```
@cartant
Copy link
Collaborator Author

cartant commented Feb 19, 2021

@benlesh LGTM. I'd approve it, but I can't approve my own PR. 🙂

@benlesh benlesh merged commit 1aa400a into ReactiveX:master Feb 19, 2021
@cartant cartant deleted the pr-5932-for-v7 branch February 19, 2021 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants