diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 273ddd15414b51..b518280b912db9 100644 --- a/lib/internal/http2/core.js +++ b/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); @@ -2468,11 +2480,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; diff --git a/test/parallel/test-http2-client-cancel-stream-after-end.js b/test/parallel/test-http2-client-cancel-stream-after-end.js new file mode 100644 index 00000000000000..a5058d31505a8f --- /dev/null +++ b/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(); + })); +} diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index ff98c23e864f74..6d4eb11e317b3b 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/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'); + })); })); } diff --git a/test/parallel/test-http2-client-jsstream-destroy.js b/test/parallel/test-http2-client-jsstream-destroy.js index 05c41efb104873..42302ec463a525 100644 --- a/test/parallel/test-http2-client-jsstream-destroy.js +++ b/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(); diff --git a/test/parallel/test-http2-client-socket-destroy.js b/test/parallel/test-http2-client-socket-destroy.js index 1c0fa54f11c326..bc72c02b6ab9d4 100644 --- a/test/parallel/test-http2-client-socket-destroy.js +++ b/test/parallel/test-http2-client-socket-destroy.js @@ -26,6 +26,7 @@ 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 @@ -33,7 +34,7 @@ server.listen(0, common.mustCall(function() { })); 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 diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index 1babb92a5ec45c..e8486920492c61 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/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(); })); diff --git a/test/parallel/test-http2-server-shutdown-options-errors.js b/test/parallel/test-http2-server-shutdown-options-errors.js index 5a2ca62a6c8e31..f193145f3ea304 100644 --- a/test/parallel/test-http2-server-shutdown-options-errors.js +++ b/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(); diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js index 4e540e31496668..54b5bb3dcfa12a 100644 --- a/test/parallel/test-http2-server-stream-session-destroy.js +++ b/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()))); })); diff --git a/test/parallel/test-http2-zero-length-header.js b/test/parallel/test-http2-zero-length-header.js index 170b8b0f1521b3..fa9de63413344f 100644 --- a/test/parallel/test-http2-zero-length-header.js +++ b/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()); }));