http2: emit error on canceled streams when aborted event is not emitted

When a client HTTP/2 stream's writable side is already ended (e.g.
GET requests), receiving RST code 8 (NGHTTP2_CANCEL) emitted neither
the 'aborted' nor the 'error' event, causing the stream to close
silently. This happened because the 'aborted' event is only emitted
when the writable side is still open, but the NGHTTP2_CANCEL code was
unconditionally excluded from error generation assuming the 'aborted'
event would cover it.

Fix by only skipping error generation for NGHTTP2_CANCEL when the
'aborted' event was actually emitted.

Fixes: #56627
Signed-off-by: Matteo Collina <hello@matteocollina.com>

Author: mcollina

lib/internal/http2/core.js

@@ -2409,8 +2409,20 @@ class Http2Stream extends Duplex {
       validateFunction(callback, 'callback');
     }
 
-    if (this.closed)
+    if (this.closed) {
+      // A client stream may already have been marked as closed with
+      // NGHTTP2_NO_ERROR by the time the session or underlying socket is
+      // canceled. Preserve the cancelation code so _destroy() can still emit
+      // the expected stream error when the aborted event was not emitted.
+      if (code === NGHTTP2_CANCEL &&
+          this[kSession] !== undefined &&
+          this[kSession][kType] === NGHTTP2_SESSION_CLIENT &&
+          this.rstCode === NGHTTP2_NO_ERROR &&
+          !this.aborted) {
+        this[kState].rstCode = code;
+      }
       return;
+    }
 
     if (callback !== undefined)
       this.once('close', callback);
@@ -2441,6 +2453,14 @@ class Http2Stream extends Duplex {
     // Previously, this always overrode a successful close operation code
     // NGHTTP2_NO_ERROR (0) with sessionCode because the use of the || operator.
     let code = this.closed ? this.rstCode : sessionCode;
+    if (this.closed &&
+        code === NGHTTP2_NO_ERROR &&
+        sessionCode === NGHTTP2_CANCEL &&
+        session[kType] === NGHTTP2_SESSION_CLIENT &&
+        !this.aborted) {
+      code = NGHTTP2_CANCEL;
+      state.rstCode = code;
+    }
     if (err != null) {
       if (sessionCode) {
         code = sessionCode;
@@ -2468,11 +2488,17 @@ class Http2Stream extends Duplex {
     sessionState.writeQueueSize -= state.writeQueueSize;
     state.writeQueueSize = 0;
 
-    // RST code 8 not emitted as an error as its used by clients to signify
-    // abort and is already covered by aborted event, also allows more
-    // seamless compatibility with http1
-    if (err == null && code !== NGHTTP2_NO_ERROR && code !== NGHTTP2_CANCEL)
+    // RST code 8 is commonly used by clients to signify abort and is already
+    // covered by the aborted event, which also keeps better compatibility with
+    // http1.
+    // However, if the aborted event was not emitted (e.g. because the
+    // writable side was already ended), client streams must still report the
+    // cancelation as an error.
+    if (err == null && code !== NGHTTP2_NO_ERROR &&
+        (code !== NGHTTP2_CANCEL ||
+         session[kType] === NGHTTP2_SESSION_CLIENT && !this.aborted)) {
       err = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code);
+    }
 
     this[kSession] = undefined;
     this[kHandle] = undefined;
@@ -3563,10 +3589,13 @@ function socketOnClose() {
     debugSessionObj(session, 'socket closed');
     const err = session.connecting ? new ERR_SOCKET_CLOSED() : null;
     const state = session[kState];
+    const code = session[kType] === NGHTTP2_SESSION_CLIENT ?
+      NGHTTP2_CANCEL :
+      NGHTTP2_NO_ERROR;
     state.streams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
     state.pendingStreams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
     session.close();
-    closeSession(session, NGHTTP2_NO_ERROR, err);
+    closeSession(session, code, err);
   }
 }
 

test/parallel/test-http2-client-cancel-stream-after-end.js

@@ -0,0 +1,43 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+
+// Regression test for https://github.com/nodejs/node/issues/56627
+// When a client stream's writable side is already ended (e.g. GET request)
+// and the server destroys the session, the client stream should emit an
+// error event with RST code NGHTTP2_CANCEL, since the 'aborted' event
+// cannot be emitted when the writable side is already ended.
+
+{
+  const server = h2.createServer();
+  server.on('stream', common.mustCall((stream) => {
+    stream.session.destroy();
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    const client = h2.connect(`http://localhost:${server.address().port}`);
+
+    client.on('close', common.mustCall(() => {
+      server.close();
+    }));
+
+    const req = client.request();
+
+    req.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
+      assert.match(err.message, /NGHTTP2_CANCEL/);
+    }));
+
+    req.on('aborted', common.mustNotCall());
+
+    req.on('close', common.mustCall(() => {
+      assert.strictEqual(req.rstCode, h2.constants.NGHTTP2_CANCEL);
+    }));
+
+    req.resume();
+  }));
+}

test/parallel/test-http2-client-destroy.js

@@ -118,7 +118,10 @@ const { listenerCount } = require('events');
       client.destroy();
     });
 
-    client.request();
+    const req = client.request();
+    req.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_ERROR');
+    }));
   }));
 }
 

test/parallel/test-http2-client-jsstream-destroy.js

@@ -45,6 +45,7 @@ server.listen(0, common.mustCall(function() {
       createConnection: () => proxy
     });
     const req = client.request();
+    req.on('error', common.mustCall());
 
     server.on('request', () => {
       socket.destroy();

test/parallel/test-http2-client-socket-destroy.js

@@ -26,14 +26,15 @@ server.on('stream', common.mustCall((stream) => {
 server.listen(0, common.mustCall(function() {
   const client = h2.connect(`http://localhost:${this.address().port}`);
   const req = client.request();
+  req.on('error', common.mustCall());
 
   req.on('response', common.mustCall(() => {
     // Send a premature socket close
     client[kSocket].destroy();
   }));
 
   req.resume();
-  req.on('end', common.mustCall());
+  req.on('end', common.mustNotCall());
   req.on('close', common.mustCall(() => server.close()));
 
   // On the client, the close event must call

test/parallel/test-http2-respond-with-file-connection-abort.js

@@ -13,7 +13,12 @@ const {
 
 const server = http2.createServer();
 server.on('stream', common.mustCall((stream) => {
-  stream.on('error', common.mustCallAtLeast((err) => assert.strictEqual(err.code, 'ECONNRESET'), 0));
+  stream.on('error', common.mustCallAtLeast((err) => {
+    assert.ok(
+      err.code === 'ECONNRESET' || err.code === 'ERR_HTTP2_STREAM_ERROR',
+      `Unexpected error code: ${err.code}`
+    );
+  }, 0));
   stream.respondWithFile(process.execPath, {
     [HTTP2_HEADER_CONTENT_TYPE]: 'application/octet-stream'
   });
@@ -22,11 +27,11 @@ server.on('stream', common.mustCall((stream) => {
 server.listen(0, common.mustCall(() => {
   const client = http2.connect(`http://localhost:${server.address().port}`);
   const req = client.request();
+  req.on('error', common.mustCall());
 
   req.on('response', common.mustCall());
   req.once('data', common.mustCall(() => {
     net.Socket.prototype.destroy.call(client.socket);
     server.close();
   }));
-  req.end();
 }));

test/parallel/test-http2-server-shutdown-options-errors.js

@@ -58,6 +58,7 @@ server.listen(
   common.mustCall(() => {
     const client = http2.connect(`http://localhost:${server.address().port}`);
     const req = client.request();
+    req.on('error', common.mustCall());
     req.resume();
     req.on('close', common.mustCall(() => {
       client.close();

test/parallel/test-http2-server-stream-session-destroy.js

@@ -52,7 +52,8 @@ server.on('stream', common.mustCall((stream) => {
 server.listen(0, common.mustCall(() => {
   const client = h2.connect(`http://localhost:${server.address().port}`);
   const req = client.request();
+  req.on('error', common.mustCall());
   req.resume();
-  req.on('end', common.mustCall());
+  req.on('end', common.mustNotCall());
   req.on('close', common.mustCall(() => server.close(common.mustCall())));
 }));

test/parallel/test-http2-zero-length-header.js

@@ -22,5 +22,6 @@ server.on('stream', common.mustCall((stream, headers) => {
 }));
 server.listen(0, common.mustCall(() => {
   const client = http2.connect(`http://localhost:${server.address().port}/`);
-  client.request({ ':path': '/', '': 'foo', 'bar': '' }).end();
+  const req = client.request({ ':path': '/', '': 'foo', 'bar': '' });
+  req.on('error', common.mustCall());
 }));