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(Subscription): null _parentage on unsubscribe #6352

Merged
merged 3 commits into from
May 4, 2021

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented May 4, 2021

Description:

This PR fixed the bug reported in #6351 by nulling this._parentage before the array is iterated. This was an oversight. It should have been nulled as is this._teardowns, immediately after it is destructured:

const { _teardowns } = this;
if (_teardowns) {
this._teardowns = null;

If this._parentage is not nulled, it's mutated as it's iterated and parents can be left holding onto child subscriptions that have been closed.

The PR also adds - in a separate commit - an if statement around the handling of _parentage, so that the implementation is more like what's done with _teardowns. If there's some reason for not using the if statement, we can just revert that commit.

Related issue (if exists): #6351

@cartant cartant requested a review from benlesh May 4, 2021 07:40
@cartant cartant added the 7.x Issues and PRs for version 7.x label May 4, 2021
Comment on lines +213 to +224
// When d is added to the subscriptions, it's added as a teardown. The
// length is 1 because the teardowns passed to the ctors are stored in a
// separate property.
expect((a as any)._teardowns).to.have.length(1);
expect((b as any)._teardowns).to.have.length(1);
expect((c as any)._teardowns).to.have.length(1);
d.unsubscribe();
// When d is unsubscribed, it should remove itself from each of its
// parents.
expect((a as any)._teardowns).to.have.length(0);
expect((b as any)._teardowns).to.have.length(0);
expect((c as any)._teardowns).to.have.length(0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could re-write this test as a memory leak test - using FinalizationRegistry instead of the private _teardown property - but IDK. It's fairly straightforward ATM.

@benlesh benlesh merged commit 88331d2 into ReactiveX:master May 4, 2021
@cartant cartant deleted the cartant/issue-6351 branch May 15, 2021 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x Issues and PRs for version 7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants