Skip to content

Commit

Permalink
fix(core): teardown and stop producer before complete/error
Browse files Browse the repository at this point in the history
To fix bugs such as #91 and others where adding .debug() would magically solve the bug, we need to
change the semantics around producer stop and stream completion/error. Previously, a stream would
complete first, and stop the producer after. Now, the stream stops its producer just before
completing. This was important to make the test 'Stream.prototype.flatten with map should not run
multiple executions of the inner' pass.

BREAKING CHANGE: in this version, when a stream completes or errors, its producer has already been
stopped. In previous versions, the stream first completes, propagates the complete to other
listeners and operators, and then its producer is stopped. You may barely notice this breaking
change when updating your code. Most existing code will still work like before.
  • Loading branch information
staltz committed Aug 16, 2016
1 parent 02b9f95 commit fed23f3
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 19 deletions.
14 changes: 7 additions & 7 deletions src/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1085,21 +1085,21 @@ export class Stream<T> implements InternalListener<T> {
this._err = err;
const a = this._ils;
const L = a.length;
this._x();
if (L == 1) a[0]._e(err); else {
const b = copy(a);
for (let i = 0; i < L; i++) b[i]._e(err);
}
this._x();
}

_c(): void {
const a = this._ils;
const L = a.length;
this._x();
if (L == 1) a[0]._c(); else {
const b = copy(a);
for (let i = 0; i < L; i++) b[i]._c();
}
this._x();
}

_x(): void { // tear down logic, after error or complete
Expand All @@ -1122,11 +1122,11 @@ export class Stream<T> implements InternalListener<T> {
if (ta !== NO) return ta._add(il);
const a = this._ils;
a.push(il);
if (a.length === 1) {
if (this._stopID !== NO) {
clearTimeout(this._stopID);
this._stopID = NO;
}
if (a.length > 1) return;
if (this._stopID !== NO) {
clearTimeout(this._stopID);
this._stopID = NO;
} else {
const p = this._prod;
if (p !== NO) p._start(this);
}
Expand Down
3 changes: 1 addition & 2 deletions tests/factory/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ describe('xs.merge', () => {

it('should complete properly when stopped asynchronously and restarted synchronously', (done) => {
const initial = xs.of('foo');
// debug here needed to stop MergeProducer asynchronously via _remove
const stream = xs.merge(initial).debug(() => {});
const stream = xs.merge(initial);

const noop = () => {};
stream.addListener({next: noop, error: noop, complete: noop});
Expand Down
7 changes: 4 additions & 3 deletions tests/memoryStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('MemoryStream', () => {

it('should be createable giving a custom producer object', (done) => {
const expected = [10, 20, 30];
let listenerGotEnd: boolean = false;
let producerStopped: boolean = false;

const stream = xs.createWithMemory({
start(listener: Listener<number>) {
Expand All @@ -37,7 +37,8 @@ describe('MemoryStream', () => {
stop() {
done();
assert.equal(expected.length, 0);
assert.equal(listenerGotEnd, true);
assert.equal(producerStopped, false);
producerStopped = true;
},
});

Expand All @@ -47,7 +48,7 @@ describe('MemoryStream', () => {
},
error: (err: any) => done(err),
complete: () => {
listenerGotEnd = true;
assert.equal(producerStopped, true);
},
});
});
Expand Down
18 changes: 18 additions & 0 deletions tests/operator/flatten.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,24 @@ describe('Stream.prototype.flatten', () => {
}
});
});

it('should not run multiple executions of the inner', (done) => {
const inner = xs.periodic(350).take(2).map(i => i * 100);
const outer = xs.periodic(200).take(4);
const stream = outer.map(() => inner).flatten();

const expected = [0, 100];
stream.addListener({
next: (x: number) => {
assert.equal(x, expected.shift());
},
error: (err: any) => done(err),
complete: () => {
assert.equal(expected.length, 0);
done();
}
});
});
});

describe('with filter+map fusion', () => {
Expand Down
15 changes: 8 additions & 7 deletions tests/stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('Stream', () => {

it('should be createable giving a custom producer object', (done) => {
const expected = [10, 20, 30];
let listenerGotEnd: boolean = false;
let producerStopped: boolean = false;

const producer: Producer<number> = {
start(listener: Listener<number>) {
Expand All @@ -53,7 +53,8 @@ describe('Stream', () => {
stop() {
done();
assert.equal(expected.length, 0);
assert.equal(listenerGotEnd, true);
assert.equal(producerStopped, false);
producerStopped = true;
},
};

Expand All @@ -64,7 +65,7 @@ describe('Stream', () => {
},
error: (err: any) => done(err),
complete: () => {
listenerGotEnd = true;
assert.equal(producerStopped, true);
},
});
});
Expand Down Expand Up @@ -218,7 +219,7 @@ describe('Stream', () => {
},
error: (err: any) => done(err),
complete: () => {
assert.equal(on, true);
assert.equal(on, false);
assert.equal(expected1.length, 0);
},
});
Expand All @@ -232,7 +233,7 @@ describe('Stream', () => {
},
error: (err: any) => done(err),
complete: () => {
assert.equal(on, true);
assert.equal(on, false);
assert.equal(expected2.length, 0);
},
});
Expand Down Expand Up @@ -265,7 +266,7 @@ describe('Stream', () => {
},
error: (err: any) => {
assert.equal(err, 'oops');
assert.equal(on, true);
assert.equal(on, false);
assert.equal(expected1.length, 0);
},
complete: () => {
Expand All @@ -282,7 +283,7 @@ describe('Stream', () => {
},
error: (err: any) => {
assert.equal(err, 'oops');
assert.equal(on, true);
assert.equal(on, false);
assert.equal(expected2.length, 0);
},
complete: () => {
Expand Down

0 comments on commit fed23f3

Please sign in to comment.