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: #24456
PR-URL: #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>

Author: addaleax

lib/internal/streams/end-of-stream.js

@@ -36,22 +36,24 @@ function eos(stream, opts, callback) {
 
 callback=once(callback);
 
-constws=stream._writableState;
-constrs=stream._readableState;
 letreadable=opts.readable||(opts.readable!==false&&stream.readable);
 letwritable=opts.writable||(opts.writable!==false&&stream.writable);
 
 constonlegacyfinish=()=>{
 if(!stream.writable)onfinish();
 };
 
+varwritableEnded=stream._writableState&&stream._writableState.finished;
 constonfinish=()=>{
 writable=false;
+writableEnded=true;
 if(!readable)callback.call(stream);
 };
 
+varreadableEnded=stream._readableState&&stream._readableState.endEmitted;
 constonend=()=>{
 readable=false;
+readableEnded=true;
 if(!writable)callback.call(stream);
 };
 
@@ -60,11 +62,16 @@ function eos(stream, opts, callback) {
 };
 
 constonclose=()=>{
-if(readable&&!(rs&&rs.ended)){
-returncallback.call(stream,newERR_STREAM_PREMATURE_CLOSE());
+leterr;
+if(readable&&!readableEnded){
+if(!stream._readableState||!stream._readableState.ended)
+err=newERR_STREAM_PREMATURE_CLOSE();
+returncallback.call(stream,err);
 }
-if(writable&&!(ws&&ws.ended)){
-returncallback.call(stream,newERR_STREAM_PREMATURE_CLOSE());
+if(writable&&!writableEnded){
+if(!stream._writableState||!stream._writableState.ended)
+err=newERR_STREAM_PREMATURE_CLOSE();
+returncallback.call(stream,err);
 }
 };
 
@@ -77,7 +84,7 @@ function eos(stream, opts, callback) {
 stream.on('abort',onclose);
 if(stream.req)onrequest();
 elsestream.on('request',onrequest);
-}elseif(writable&&!ws){// legacy streams
+}elseif(writable&&!stream._writableState){// legacy streams
 stream.on('end',onlegacyfinish);
 stream.on('close',onlegacyfinish);
 }

test/parallel/parallel.status

@@ -12,8 +12,6 @@ test-net-connect-options-port: PASS,FLAKY
 test-http2-pipe: PASS,FLAKY
 test-worker-syntax-error: PASS,FLAKY
 test-worker-syntax-error-file: PASS,FLAKY
-# https://github.com/nodejs/node/issues/24456
-test-stream-pipeline-http2: PASS,FLAKY
 
 [$system==linux]
 

test/parallel/test-stream-pipeline-queued-end-in-destroy.js

@@ -0,0 +1,39 @@
+'use strict';
+constcommon=require('../common');
+constassert=require('assert');
+const{ Readable, Duplex, pipeline }=require('stream');
+
+// Test that the callback for pipeline() is called even when the ._destroy()
+// method of the stream places an .end() request to itself that does not
+// get processed before the destruction of the stream (i.e. the 'close' event).
+// Refs: https://github.com/nodejs/node/issues/24456
+
+constreadable=newReadable({
+read: common.mustCall(()=>{})
+});
+
+constduplex=newDuplex({
+write(chunk,enc,cb){
+// Simulate messages queueing up.
+},
+read(){},
+destroy(err,cb){
+// Call end() from inside the destroy() method, like HTTP/2 streams
+// do at the time of writing.
+this.end();
+cb(err);
+}
+});
+
+duplex.on('finished',common.mustNotCall());
+
+pipeline(readable,duplex,common.mustCall((err)=>{
+assert.strictEqual(err.code,'ERR_STREAM_PREMATURE_CLOSE');
+}));
+
+// Write one chunk of data, and destroy the stream later.
+// That should trigger the pipeline destruction.
+readable.push('foo');
+setImmediate(()=>{
+readable.destroy();
+});