From 3500d5fa5aa62c98be752ba73bace814d889a029 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=88=E5=88=9A?= Date: Sat, 20 Jan 2018 11:06:26 +0800 Subject: [PATCH 1/4] stream: fix not calling cleanup() when unpiping all streams. Refs: https://github.com/nodejs/node/pull/12746 --- lib/_stream_readable.js | 2 +- .../test-stream-pipe-unpipe-streams.js | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 9cf786a15be847..5761ce3a28615a 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -759,7 +759,7 @@ Readable.prototype.unpipe = function(dest) { state.flowing = false; for (var i = 0; i < len; i++) - dests[i].emit('unpipe', this, unpipeInfo); + dests[i].emit('unpipe', this, { hasUnpiped: false }); return this; } diff --git a/test/parallel/test-stream-pipe-unpipe-streams.js b/test/parallel/test-stream-pipe-unpipe-streams.js index cd29b66365ae0f..431e34273e2b61 100644 --- a/test/parallel/test-stream-pipe-unpipe-streams.js +++ b/test/parallel/test-stream-pipe-unpipe-streams.js @@ -31,3 +31,26 @@ source.unpipe(dest2); source.unpipe(dest1); assert.strictEqual(source._readableState.pipes, null); + +{ + // unpipe all + const source = Readable({ read: () => {} }); + const dest1 = Writable({ write: () => {} }); + const dest2 = Writable({ write: () => {} }); + + source.pipe(dest1); + source.pipe(dest2); + + checkDestCleanup(dest1); + checkDestCleanup(dest2); + + source.unpipe(); + + function checkDestCleanup(dest) { + const unpipeChecker = common.mustCall(() => { + dest.removeListener('unpipe', unpipeChecker); + assert.strictEqual(dest.listenerCount('unpipe'), 0); + }); + dest.on('unpipe', unpipeChecker); + } +} From 020956ff0cd7b3e3f3be81c8ce810a3a400e4412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=88=E5=88=9A?= Date: Sat, 20 Jan 2018 22:04:08 +0800 Subject: [PATCH 2/4] Add more test content --- .../test-stream-pipe-unpipe-streams.js | 43 ++++++++++++++++--- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/test/parallel/test-stream-pipe-unpipe-streams.js b/test/parallel/test-stream-pipe-unpipe-streams.js index 431e34273e2b61..7807da29505746 100644 --- a/test/parallel/test-stream-pipe-unpipe-streams.js +++ b/test/parallel/test-stream-pipe-unpipe-streams.js @@ -33,24 +33,53 @@ source.unpipe(dest1); assert.strictEqual(source._readableState.pipes, null); { - // unpipe all + // test `cleanup()` if we unpipe all streams. const source = Readable({ read: () => {} }); const dest1 = Writable({ write: () => {} }); const dest2 = Writable({ write: () => {} }); - source.pipe(dest1); - source.pipe(dest2); + let destCount = 0; + const srcCheckEventNames = ['end', 'data']; + const destCheckEventNames = ['close', 'finish', 'drain', 'error', 'unpipe']; - checkDestCleanup(dest1); - checkDestCleanup(dest2); + const checkSrcCleanup = common.mustCall(() => { + assert.strictEqual(source._readableState.pipes, null); + assert.strictEqual(source._readableState.pipesCount, 0); + assert.strictEqual(source._readableState.flowing, false); - source.unpipe(); + srcCheckEventNames.forEach(eventName => { + assert.strictEqual( + source.listenerCount(eventName), 0, + `source's '${eventName}' event listeners don't be cleaned up` + ); + }); + }); function checkDestCleanup(dest) { + const currentDestId = ++destCount; + source.pipe(dest); + const unpipeChecker = common.mustCall(() => { + assert.strictEqual( + dest.listenerCount('unpipe'), 1, + `destination{${currentDestId}} should have a 'unpipe' event listener which is \`unpipeChecker\`` + ); dest.removeListener('unpipe', unpipeChecker); - assert.strictEqual(dest.listenerCount('unpipe'), 0); + destCheckEventNames.forEach(eventName => { + assert.strictEqual( + dest.listenerCount(eventName), 0, + `destination{${currentDestId}}'s '${eventName}' event listeners don't be cleaned up` + ); + }); + + if (--destCount === 0) + checkSrcCleanup(); }); + dest.on('unpipe', unpipeChecker); } + + checkDestCleanup(dest1); + checkDestCleanup(dest2); + source.unpipe(); } From 8da1b7288f65384afafd475a5a19e1ca589e4505 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=88=E5=88=9A?= Date: Sat, 20 Jan 2018 22:12:12 +0800 Subject: [PATCH 3/4] Fix lint --- test/parallel/test-stream-pipe-unpipe-streams.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-stream-pipe-unpipe-streams.js b/test/parallel/test-stream-pipe-unpipe-streams.js index 7807da29505746..6f51f8b38e67f3 100644 --- a/test/parallel/test-stream-pipe-unpipe-streams.js +++ b/test/parallel/test-stream-pipe-unpipe-streams.js @@ -33,7 +33,7 @@ source.unpipe(dest1); assert.strictEqual(source._readableState.pipes, null); { - // test `cleanup()` if we unpipe all streams. + // test `cleanup()` if we unpipe all streams. const source = Readable({ read: () => {} }); const dest1 = Writable({ write: () => {} }); const dest2 = Writable({ write: () => {} }); @@ -47,7 +47,7 @@ assert.strictEqual(source._readableState.pipes, null); assert.strictEqual(source._readableState.pipesCount, 0); assert.strictEqual(source._readableState.flowing, false); - srcCheckEventNames.forEach(eventName => { + srcCheckEventNames.forEach((eventName) => { assert.strictEqual( source.listenerCount(eventName), 0, `source's '${eventName}' event listeners don't be cleaned up` @@ -62,13 +62,15 @@ assert.strictEqual(source._readableState.pipes, null); const unpipeChecker = common.mustCall(() => { assert.strictEqual( dest.listenerCount('unpipe'), 1, - `destination{${currentDestId}} should have a 'unpipe' event listener which is \`unpipeChecker\`` + `destination{${currentDestId}} should have a 'unpipe' event ` + + 'listener which is `unpipeChecker`' ); dest.removeListener('unpipe', unpipeChecker); - destCheckEventNames.forEach(eventName => { + destCheckEventNames.forEach((eventName) => { assert.strictEqual( dest.listenerCount(eventName), 0, - `destination{${currentDestId}}'s '${eventName}' event listeners don't be cleaned up` + `destination{${currentDestId}}'s '${eventName}' event ` + + "listeners don't be cleaned up" ); }); From eba4582d6f94f23d9f15527686fccfbb9d9dc223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=88=E5=88=9A?= Date: Mon, 22 Jan 2018 08:35:07 +0800 Subject: [PATCH 4/4] Fix code style --- test/parallel/test-stream-pipe-unpipe-streams.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-stream-pipe-unpipe-streams.js b/test/parallel/test-stream-pipe-unpipe-streams.js index 6f51f8b38e67f3..49e02bea9cb695 100644 --- a/test/parallel/test-stream-pipe-unpipe-streams.js +++ b/test/parallel/test-stream-pipe-unpipe-streams.js @@ -50,7 +50,7 @@ assert.strictEqual(source._readableState.pipes, null); srcCheckEventNames.forEach((eventName) => { assert.strictEqual( source.listenerCount(eventName), 0, - `source's '${eventName}' event listeners don't be cleaned up` + `source's '${eventName}' event listeners not removed` ); }); }); @@ -60,8 +60,8 @@ assert.strictEqual(source._readableState.pipes, null); source.pipe(dest); const unpipeChecker = common.mustCall(() => { - assert.strictEqual( - dest.listenerCount('unpipe'), 1, + assert.deepStrictEqual( + dest.listeners('unpipe'), [unpipeChecker], `destination{${currentDestId}} should have a 'unpipe' event ` + 'listener which is `unpipeChecker`' ); @@ -70,7 +70,7 @@ assert.strictEqual(source._readableState.pipes, null); assert.strictEqual( dest.listenerCount(eventName), 0, `destination{${currentDestId}}'s '${eventName}' event ` + - "listeners don't be cleaned up" + 'listeners not removed' ); });