stream: do not emit end on readable error

szmarczak · nodejs-github-bot · commit ef992f6de98e · 2021-08-05T09:58:01.000Z
PR-URL: #39607 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js
@@ -1328,7 +1328,7 @@ function endReadableNT(state, stream) {
   debug('endReadableNT', state.endEmitted, state.length);
 
   // Check that we didn't get one last unshift.
-  if (!state.errorEmitted && !state.closeEmitted &&
+  if (!state.errored && !state.closeEmitted &&
       !state.endEmitted && state.length === 0) {
     state.endEmitted = true;
     stream.emit('end');
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
@@ -163,7 +163,7 @@ const { getEventListeners } = require('events');
 
     client.close();
     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-onconnect-errors.js b/test/parallel/test-http2-client-onconnect-errors.js
@@ -102,7 +102,7 @@ function runTest(test) {
     });
   }
 
-  req.on('end', common.mustCall());
+  req.on('end', common.mustNotCall());
   req.on('close', common.mustCall(() => {
     client.destroy();
 
diff --git a/test/parallel/test-http2-compat-serverresponse-destroy.js b/test/parallel/test-http2-compat-serverresponse-destroy.js
@@ -62,7 +62,7 @@ server.listen(0, common.mustCall(() => {
     req.on('close', common.mustCall(() => countdown.dec()));
 
     req.resume();
-    req.on('end', common.mustCall());
+    req.on('end', common.mustNotCall());
   }
 
   {
@@ -77,6 +77,6 @@ server.listen(0, common.mustCall(() => {
     req.on('close', common.mustCall(() => countdown.dec()));
 
     req.resume();
-    req.on('end', common.mustCall());
+    req.on('end', common.mustNotCall());
   }
 }));
diff --git a/test/parallel/test-http2-empty-frame-without-eof.js b/test/parallel/test-http2-empty-frame-without-eof.js
@@ -34,7 +34,9 @@ async function main() {
       client.on('error', common.expectsError(expected));
     }
     stream.resume();
-    await once(stream, 'end');
+    await new Promise((resolve) => {
+      stream.once('close', resolve);
+    });
     client.close();
   }
   server.close();
diff --git a/test/parallel/test-http2-max-concurrent-streams.js b/test/parallel/test-http2-max-concurrent-streams.js
@@ -45,7 +45,7 @@ server.listen(0, common.mustCall(() => {
     req.on('aborted', common.mustCall());
     req.on('response', common.mustNotCall());
     req.resume();
-    req.on('end', common.mustCall());
+    req.on('end', common.mustNotCall());
     req.on('close', common.mustCall(() => countdown.dec()));
     req.on('error', common.expectsError({
       code: 'ERR_HTTP2_STREAM_ERROR',
diff --git a/test/parallel/test-http2-multi-content-length.js b/test/parallel/test-http2-multi-content-length.js
@@ -54,7 +54,7 @@ server.listen(0, common.mustCall(() => {
     // header to be set for non-payload bearing requests...
     const req = client.request({ 'content-length': 1 });
     req.resume();
-    req.on('end', common.mustCall());
+    req.on('end', common.mustNotCall());
     req.on('close', common.mustCall(() => countdown.dec()));
     req.on('error', common.expectsError({
       code: 'ERR_HTTP2_STREAM_ERROR',
diff --git a/test/parallel/test-http2-respond-file-fd-invalid.js b/test/parallel/test-http2-respond-file-fd-invalid.js
@@ -40,7 +40,8 @@ server.listen(0, () => {
   req.on('response', common.mustCall());
   req.on('error', errorCheck);
   req.on('data', common.mustNotCall());
-  req.on('end', common.mustCall(() => {
+  req.on('end', common.mustNotCall());
+  req.on('close', common.mustCall(() => {
     assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);
     client.close();
     server.close();
diff --git a/test/parallel/test-http2-respond-nghttperrors.js b/test/parallel/test-http2-respond-nghttperrors.js
@@ -83,12 +83,13 @@ function runTest(test) {
     name: 'Error',
     message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR'
   }));
+  req.on('end', common.mustNotCall());
 
   currentError = test;
   req.resume();
   req.end();
 
-  req.on('end', common.mustCall(() => {
+  req.on('close', common.mustCall(() => {
     client.close();
 
     if (!tests.length) {
diff --git a/test/parallel/test-http2-respond-with-fd-errors.js b/test/parallel/test-http2-respond-with-fd-errors.js
@@ -91,12 +91,13 @@ function runTest(test) {
     name: 'Error',
     message: 'Stream closed with error code NGHTTP2_INTERNAL_ERROR'
   }));
+  req.on('end', common.mustNotCall());
 
   currentError = test;
   req.resume();
   req.end();
 
-  req.on('end', common.mustCall(() => {
+  req.on('close', common.mustCall(() => {
     client.close();
 
     if (!tests.length) {
diff --git a/test/parallel/test-http2-server-shutdown-before-respond.js b/test/parallel/test-http2-server-shutdown-before-respond.js
@@ -32,5 +32,6 @@ server.on('listening', common.mustCall(() => {
   }));
   req.resume();
   req.on('data', common.mustNotCall());
-  req.on('end', common.mustCall(() => server.close()));
+  req.on('end', common.mustNotCall());
+  req.on('close', common.mustCall(() => server.close()));
 }));
diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js
@@ -8,6 +8,7 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const h2 = require('http2');
 const { kSocket } = require('internal/http2/util');
+const { once } = require('events');
 
 const server = h2.createServer();
 
@@ -36,7 +37,7 @@ function onStream(stream) {
 
 server.listen(0);
 
-server.on('listening', common.mustCall(() => {
+server.on('listening', common.mustCall(async () => {
   const client = h2.connect(`http://localhost:${server.address().port}`);
   // The client may have an ECONNRESET error here depending on the operating
   // system, due mainly to differences in the timing of socket closing. Do
@@ -58,5 +59,8 @@ server.on('listening', common.mustCall(() => {
 
   req.on('aborted', common.mustCall());
   req.resume();
-  req.on('end', common.mustCall());
+
+  try {
+    await once(req, 'end');
+  } catch {}
 }));

Original file line number	Diff line number	Diff line change
`@@ -163,7 +163,7 @@ const { getEventListeners } = require('events');`
`163`	`163`
`164`	`164`	`client.close();`
`165`	`165`	`req.resume();`
`166`		`- req.on('end', common.mustCall());`
	`166`	`+ req.on('end', common.mustNotCall());`
`167`	`167`	`req.on('close', common.mustCall(() => server.close()));`
`168`	`168`	`}));`
`169`	`169`	`}`
Original file line number	Diff line number	Diff line change
`@@ -102,7 +102,7 @@ function runTest(test) {`
`102`	`102`	`});`
`103`	`103`	`}`
`104`	`104`
`105`		`- req.on('end', common.mustCall());`
	`105`	`+ req.on('end', common.mustNotCall());`
`106`	`106`	`req.on('close', common.mustCall(() => {`
`107`	`107`	`client.destroy();`
`108`	`108`
Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ server.listen(0, common.mustCall(() => {`
`62`	`62`	`req.on('close', common.mustCall(() => countdown.dec()));`
`63`	`63`
`64`	`64`	`req.resume();`
`65`		`- req.on('end', common.mustCall());`
	`65`	`+ req.on('end', common.mustNotCall());`
`66`	`66`	`}`
`67`	`67`
`68`	`68`	`{`
`@@ -77,6 +77,6 @@ server.listen(0, common.mustCall(() => {`
`77`	`77`	`req.on('close', common.mustCall(() => countdown.dec()));`
`78`	`78`
`79`	`79`	`req.resume();`
`80`		`- req.on('end', common.mustCall());`
	`80`	`+ req.on('end', common.mustNotCall());`
`81`	`81`	`}`
`82`	`82`	`}));`
Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,9 @@ async function main() {`
`34`	`34`	`client.on('error', common.expectsError(expected));`
`35`	`35`	`}`
`36`	`36`	`stream.resume();`
`37`		`- await once(stream, 'end');`
	`37`	`+ await new Promise((resolve) => {`
	`38`	`+ stream.once('close', resolve);`
	`39`	`+ });`
`38`	`40`	`client.close();`
`39`	`41`	`}`
`40`	`42`	`server.close();`