diff options
author | Anna Henningsen <anna@addaleax.net> | 2018-12-09 20:05:11 +0100 |
---|---|---|
committer | Rich Trott <rtrott@gmail.com> | 2018-12-11 14:21:52 -0800 |
commit | 83ec33b9335a7140c1f8b46357303ff7a8122a0d (patch) | |
tree | 40bc5b049e161560e0afe6260d622277df30b2b5 /lib/internal/streams | |
parent | 2f75eed1aa50ee1ce3d3cdd47bb4108ff5de4679 (diff) | |
download | android-node-v8-83ec33b9335a7140c1f8b46357303ff7a8122a0d.tar.gz android-node-v8-83ec33b9335a7140c1f8b46357303ff7a8122a0d.tar.bz2 android-node-v8-83ec33b9335a7140c1f8b46357303ff7a8122a0d.zip |
stream: fix end-of-stream for HTTP/2
HTTP/2 streams call `.end()` on themselves from their
`.destroy()` method, which might be queued (e.g. due to network
congestion) and not processed before the stream itself is destroyed.
In that case, the `_writableState.ended` property could be set before
the stream emits its `'close'` event, and never actually emits the
`'finished'` event, confusing the end-of-stream implementation so
that it wouldn’t call its callback.
This can be fixed by watching for the end events themselves using the
existing `'finish'` and `'end'` listeners rather than relying on the
`.ended` properties of the `_...State` objects.
These properties still need to be checked to know whether stream
closure was premature – My understanding is that ideally, streams
should not emit `'close'` before `'end'` and/or `'finished'`, so this
might be another bug, but changing this would require modifying tests
and almost certainly be a breaking change.
Fixes: https://github.com/nodejs/node/issues/24456
PR-URL: https://github.com/nodejs/node/pull/24926
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Diffstat (limited to 'lib/internal/streams')
-rw-r--r-- | lib/internal/streams/end-of-stream.js | 21 |
1 files changed, 14 insertions, 7 deletions
diff --git a/lib/internal/streams/end-of-stream.js b/lib/internal/streams/end-of-stream.js index 8bbd4827b2..4ad7b93337 100644 --- a/lib/internal/streams/end-of-stream.js +++ b/lib/internal/streams/end-of-stream.js @@ -36,8 +36,6 @@ function eos(stream, opts, callback) { callback = once(callback); - const ws = stream._writableState; - const rs = stream._readableState; let readable = opts.readable || (opts.readable !== false && stream.readable); let writable = opts.writable || (opts.writable !== false && stream.writable); @@ -45,13 +43,17 @@ function eos(stream, opts, callback) { if (!stream.writable) onfinish(); }; + var writableEnded = stream._writableState && stream._writableState.finished; const onfinish = () => { writable = false; + writableEnded = true; if (!readable) callback.call(stream); }; + var readableEnded = stream._readableState && stream._readableState.endEmitted; const onend = () => { readable = false; + readableEnded = true; if (!writable) callback.call(stream); }; @@ -60,11 +62,16 @@ function eos(stream, opts, callback) { }; const onclose = () => { - if (readable && !(rs && rs.ended)) { - return callback.call(stream, new ERR_STREAM_PREMATURE_CLOSE()); + let err; + if (readable && !readableEnded) { + if (!stream._readableState || !stream._readableState.ended) + err = new ERR_STREAM_PREMATURE_CLOSE(); + return callback.call(stream, err); } - if (writable && !(ws && ws.ended)) { - return callback.call(stream, new ERR_STREAM_PREMATURE_CLOSE()); + if (writable && !writableEnded) { + if (!stream._writableState || !stream._writableState.ended) + err = new ERR_STREAM_PREMATURE_CLOSE(); + return callback.call(stream, err); } }; @@ -77,7 +84,7 @@ function eos(stream, opts, callback) { stream.on('abort', onclose); if (stream.req) onrequest(); else stream.on('request', onrequest); - } else if (writable && !ws) { // legacy streams + } else if (writable && !stream._writableState) { // legacy streams stream.on('end', onlegacyfinish); stream.on('close', onlegacyfinish); } |