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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 26 additions & 3 deletions spec/Subscription-spec.ts
Expand Up @@ -116,7 +116,7 @@ describe('Subscription', () => {
});

describe('unsubscribe()', () => {
it('Should unsubscribe from all subscriptions, when some of them throw', (done) => {
it('should unsubscribe from all subscriptions, when some of them throw', (done) => {
const tearDowns: number[] = [];

const source1 = new Observable(() => {
Expand Down Expand Up @@ -149,7 +149,7 @@ describe('Subscription', () => {
});
});

it('Should unsubscribe from all subscriptions, when adding a bad custom subscription to a subscription', (done) => {
it('should unsubscribe from all subscriptions, when adding a bad custom subscription to a subscription', (done) => {
const tearDowns: number[] = [];

const sub = new Subscription();
Expand Down Expand Up @@ -189,7 +189,7 @@ describe('Subscription', () => {
});
});

it('Should have idempotent unsubscription', () => {
it('should have idempotent unsubscription', () => {
let count = 0;
const subscription = new Subscription(() => ++count);
expect(count).to.equal(0);
Expand All @@ -200,5 +200,28 @@ describe('Subscription', () => {
subscription.unsubscribe();
expect(count).to.equal(1);
});

it('should unsubscribe from all parents', () => {
// https://github.com/ReactiveX/rxjs/issues/6351
const a = new Subscription(() => { /* noop */});
const b = new Subscription(() => { /* noop */});
const c = new Subscription(() => { /* noop */});
const d = new Subscription(() => { /* noop */});
a.add(d);
b.add(d);
c.add(d);
// 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);
Comment on lines +213 to +224
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.

});
});
});
13 changes: 8 additions & 5 deletions src/internal/Subscription.ts
Expand Up @@ -56,12 +56,15 @@ export class Subscription implements SubscriptionLike {

// Remove this from it's parents.
const { _parentage } = this;
if (Array.isArray(_parentage)) {
for (const parent of _parentage) {
parent.remove(this);
if (_parentage) {
this._parentage = null;
if (Array.isArray(_parentage)) {
for (const parent of _parentage) {
parent.remove(this);
}
} else {
_parentage.remove(this);
}
} else {
_parentage?.remove(this);
}

const { initialTeardown } = this;
Expand Down