From 8ffa2e98dfd84e7b7b852374979e4311f95dd92e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 3 Jan 2020 21:41:44 +0100 Subject: [PATCH 1/6] stream: don't emit 'end' after 'error' Refs: https://github.com/nodejs/node/issues/6083 --- lib/_stream_readable.js | 2 +- lib/internal/streams/destroy.js | 2 ++ .../test-stream-readable-error-end.js | 26 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-stream-readable-error-end.js diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 6ca5feede65b4b..07ce62549b3147 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -1214,7 +1214,7 @@ function endReadableNT(state, stream) { debug('endReadableNT', state.endEmitted, state.length); // Check that we didn't get one last unshift. - if (!state.endEmitted && state.length === 0) { + if (!state.errorEmitted && !state.endEmitted && state.length === 0) { state.endEmitted = true; stream.readable = false; stream.emit('end'); diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index fb206c6c83d0a0..c5d9ddeec3a6a1 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -16,6 +16,8 @@ function destroy(err, cb) { } } + // TODO(ronag): readable & writable = false? + if ((w && w.destroyed) || (r && r.destroyed)) { if (cb) { cb(err); diff --git a/test/parallel/test-stream-readable-error-end.js b/test/parallel/test-stream-readable-error-end.js new file mode 100644 index 00000000000000..ed28de2ed5d77a --- /dev/null +++ b/test/parallel/test-stream-readable-error-end.js @@ -0,0 +1,26 @@ +'use strict'; + +const common = require('../common'); +const { Readable } = require('stream'); +const assert = require('assert'); + +{ + // Ensure 'end' is not emitted after 'error'. + // This test is slightly more complicated than + // needed in order to better illustrate the invariant. + + const r = new Readable({ read() {} }); + + let errorEmitted = false; + + r.on('data', common.mustCall()); + r.on('end', () => { + assert.strictEqual(!errorEmitted); + }); + r.on('error', () => { + errorEmitted = true; + }); + r.push('asd'); + r.push(null); + r.destroy(new Error('kaboom')); +} From 73c35c2fcc3f2385ad29b1f20742ecba22f7a3b5 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 3 Jan 2020 22:34:07 +0100 Subject: [PATCH 2/6] fixup: http2 --- test/parallel/test-http2-client-destroy.js | 2 +- .../parallel/test-http2-client-stream-destroy-before-connect.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index 88850b9db51ccc..12da903c6535a0 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -95,7 +95,7 @@ const Countdown = require('../common/countdown'); }); req.resume(); - req.on('end', common.mustCall()); + req.on('end', common.mustNotCall()); req.on('close', common.mustCall(() => server.close())); })); } diff --git a/test/parallel/test-http2-client-stream-destroy-before-connect.js b/test/parallel/test-http2-client-stream-destroy-before-connect.js index 09667750ae16fd..087b06d01bd837 100644 --- a/test/parallel/test-http2-client-stream-destroy-before-connect.js +++ b/test/parallel/test-http2-client-stream-destroy-before-connect.js @@ -50,5 +50,5 @@ server.listen(0, common.mustCall(() => { req.on('response', common.mustNotCall()); req.resume(); - req.on('end', common.mustCall()); + req.on('end', common.mustNotCall()); })); From 57430da09c73088aa65c7cbba1b28b6886bb1707 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 3 Jan 2020 22:39:03 +0100 Subject: [PATCH 3/6] fixup: unshift test --- test/parallel/test-stream-unshift-read-race.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-stream-unshift-read-race.js b/test/parallel/test-stream-unshift-read-race.js index 05b8765062455c..fe110ea285521e 100644 --- a/test/parallel/test-stream-unshift-read-race.js +++ b/test/parallel/test-stream-unshift-read-race.js @@ -68,6 +68,9 @@ r._read = function(n) { }; function pushError() { + r.unshift(Buffer.allocUnsafe(1)); + w.end(); + assert.throws(() => { r.push(Buffer.allocUnsafe(1)); }, { @@ -85,10 +88,7 @@ w._write = function(chunk, encoding, cb) { cb(); }; -r.on('end', common.mustCall(function() { - r.unshift(Buffer.allocUnsafe(1)); - w.end(); -})); +r.on('end', common.mustNotCall()); r.on('readable', function() { let chunk; From c23b4022943afed2a57c06d7cb001b611d92d0ee Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 4 Jan 2020 00:05:45 +0100 Subject: [PATCH 4/6] fixup: http2 --- test/parallel/test-http2-head-request.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/parallel/test-http2-head-request.js b/test/parallel/test-http2-head-request.js index 9eaa737503b377..6cf0a4d81cd41c 100644 --- a/test/parallel/test-http2-head-request.js +++ b/test/parallel/test-http2-head-request.js @@ -10,7 +10,7 @@ const errCheck = common.expectsError({ name: 'Error', code: 'ERR_STREAM_WRITE_AFTER_END', message: 'write after end' -}, 2); +}, 1); const { HTTP2_HEADER_PATH, @@ -41,12 +41,6 @@ server.listen(0, () => { [HTTP2_HEADER_PATH]: '/' }); - // Because it is a HEAD request, the payload is meaningless. The - // option.endStream flag is set automatically making the stream - // non-writable. - req.on('error', errCheck); - req.write('data'); - req.on('response', common.mustCall((headers, flags) => { assert.strictEqual(headers[HTTP2_HEADER_STATUS], 200); assert.strictEqual(flags, 5); // The end of stream flag is set From 4c7193e04396963b0f943b197444a39342901085 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 4 Jan 2020 11:24:50 +0100 Subject: [PATCH 5/6] fixup: simplify test --- test/parallel/test-stream-readable-error-end.js | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/test/parallel/test-stream-readable-error-end.js b/test/parallel/test-stream-readable-error-end.js index ed28de2ed5d77a..90e21bb79d6f79 100644 --- a/test/parallel/test-stream-readable-error-end.js +++ b/test/parallel/test-stream-readable-error-end.js @@ -5,21 +5,11 @@ const { Readable } = require('stream'); const assert = require('assert'); { - // Ensure 'end' is not emitted after 'error'. - // This test is slightly more complicated than - // needed in order to better illustrate the invariant. - const r = new Readable({ read() {} }); - let errorEmitted = false; - r.on('data', common.mustCall()); - r.on('end', () => { - assert.strictEqual(!errorEmitted); - }); - r.on('error', () => { - errorEmitted = true; - }); + r.on('end', common.mustNotCall()); + r.on('error', common.mustCall()); r.push('asd'); r.push(null); r.destroy(new Error('kaboom')); From 9750a0b6339250bb4276c20c2711b1dd830652f6 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 4 Jan 2020 11:25:51 +0100 Subject: [PATCH 6/6] fixup: TODO --- lib/internal/streams/destroy.js | 6 ++++-- test/parallel/test-stream-readable-error-end.js | 3 +-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index c5d9ddeec3a6a1..c70b9283290f0d 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -7,6 +7,8 @@ function destroy(err, cb) { const r = this._readableState; const w = this._writableState; + // TODO(ronag): readable & writable = false? + if (err) { if (w) { w.errored = true; @@ -16,8 +18,6 @@ function destroy(err, cb) { } } - // TODO(ronag): readable & writable = false? - if ((w && w.destroyed) || (r && r.destroyed)) { if (cb) { cb(err); @@ -131,6 +131,8 @@ function errorOrDestroy(stream, err) { const r = stream._readableState; const w = stream._writableState; + // TODO(ronag): readable & writable = false? + if ((r && r.autoDestroy) || (w && w.autoDestroy)) stream.destroy(err); else if (err) { diff --git a/test/parallel/test-stream-readable-error-end.js b/test/parallel/test-stream-readable-error-end.js index 90e21bb79d6f79..b46fd7f32c6bd9 100644 --- a/test/parallel/test-stream-readable-error-end.js +++ b/test/parallel/test-stream-readable-error-end.js @@ -2,13 +2,12 @@ const common = require('../common'); const { Readable } = require('stream'); -const assert = require('assert'); { const r = new Readable({ read() {} }); - r.on('data', common.mustCall()); r.on('end', common.mustNotCall()); + r.on('data', common.mustCall()); r.on('error', common.mustCall()); r.push('asd'); r.push(null);