Skip to content

Commit

Permalink
fix(Subscription): null _parentage on unsubscribe (#6352)
Browse files Browse the repository at this point in the history
* test: add failing test for #6351

* fix(Subscription): null _parentage on unsubscribe

Closes #6351

* refactor: add if statement around _parentage
  • Loading branch information
cartant authored May 4, 2021
1 parent 7b30546 commit 88331d2
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 8 deletions.
29 changes: 26 additions & 3 deletions spec/Subscription-spec.ts
Original file line number Diff line number Diff line change
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);
});
});
});
13 changes: 8 additions & 5 deletions src/internal/Subscription.ts
Original file line number Diff line number Diff line change
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

0 comments on commit 88331d2

Please sign in to comment.