http2: error for incomplete reads on RST, auto-drain, deprecate aborted

Signed-off-by: Tim Perry <pimterry@gmail.com>
PR-URL: #63249
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

Author: pimterry

doc/api/deprecations.md

@@ -4574,7 +4574,62 @@ throwing an error. This behavior is inconsistent with `hash.digest()` and
 may lead to subtle bugs. Calling `hmac.digest()` on a finalized `Hmac` instance
 will throw an error in a future version.
 
+### DEP0207: `.aborted` property and `'aborted'` event in `http2`
+
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/63249
+    description: Documentation-only deprecation.
+-->
+
+Type: Documentation-only
+
+Use standard stream events and state checks instead. Read-side aborts
+(peer cancelled before sending `END_STREAM`) now surface as `'error'`
+with code `ERR_HTTP2_STREAM_ABORTED` (clean peer reset code) or
+`ERR_HTTP2_STREAM_ERROR` (non-clean code). Write-side aborts (peer
+cancelled while we still had writes in flight) are detectable from
+`'close'` by checking `writableFinished`. Parallels [DEP0156][] for
+`http`.
+
+```cjs
+// Deprecated
+server.on('stream', (stream) => {
+  stream.on('aborted', () => {
+    // Stream was closed while the writable was still open.
+  });
+});
+```
+
+```cjs
+// Use this instead
+server.on('stream', (stream) => {
+  // Read-side abort: peer cancelled before sending END_STREAM.
+  stream.on('error', (err) => {
+    if (err.code === 'ERR_HTTP2_STREAM_ABORTED' ||
+        err.code === 'ERR_HTTP2_STREAM_ERROR') {
+      // Peer cancelled the request mid-stream.
+    }
+  });
+  // Write-side abort: our response didn't fully send before close.
+  stream.on('close', () => {
+    if (!stream.writableFinished) {
+      // Writes were aborted (peer cancel, local destroy, etc.).
+    }
+  });
+});
+```
+
+The same patterns apply to the compatibility API (`req` / `res` on
+`http2.createServer((req, res) => …)`). On the read-side, errors on the
+underlying stream are emitted from `req`. On the write-side you can use
+`res.on('close', …)` to hear about client aborts by checking
+`res.writableFinished` to confirm whether the response was written
+successfully before the response closed.
+
 [DEP0142]: #dep0142-repl_builtinlibs
+[DEP0156]: #dep0156-aborted-property-and-abort-aborted-event-in-http
 [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
 [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3
 [RFC 8247 Section 2.4]: https://www.rfc-editor.org/rfc/rfc8247#section-2.4

doc/api/errors.md

@@ -1718,6 +1718,15 @@ Use of the `101` Informational status code is forbidden in HTTP/2.
 An invalid HTTP status code has been specified. Status codes must be an integer
 between `100` and `599` (inclusive).
 
+<a id="ERR_HTTP2_STREAM_ABORTED"></a>
+
+### `ERR_HTTP2_STREAM_ABORTED`
+
+The peer reset the `Http2Stream` with a clean error code (`NGHTTP2_NO_ERROR`
+or `NGHTTP2_CANCEL`) before sending `END_STREAM`, so the readable side will
+not be fully delivered. Mirrors HTTP/1's `ECONNRESET` for a peer-side
+`socket.destroy()`.
+
 <a id="ERR_HTTP2_STREAM_CANCEL"></a>
 
 ### `ERR_HTTP2_STREAM_CANCEL`

doc/api/http2.md

@@ -1238,22 +1238,23 @@ the value is `undefined`, the stream is not yet ready for use.
 
 ##### Destruction
 
-All [`Http2Stream`][] instances are destroyed either when:
+All [`Http2Stream`][] instances are destroyed when one of the following
+happens:
 
-* An `RST_STREAM` frame for the stream is received by the connected peer,
-  and (for client streams only) pending data has been read.
-* The `http2stream.close()` method is called, and (for client streams only)
-  pending data has been read.
-* The `http2stream.destroy()` or `http2session.destroy()` methods are called.
+* Both sides send `END_STREAM` (a clean exchange).
+* The peer sends an `RST_STREAM` frame.
+* `http2stream.close()`, `http2stream.destroy()`, or `http2session.destroy()`
+  is called locally.
 
-When an `Http2Stream` instance is destroyed, an attempt will be made to send an
-`RST_STREAM` frame to the connected peer.
+For clean exchanges and clean cancels, the destroy is deferred until any
+pending `'end'` and `'finish'` events have fired. When destroyed, an
+attempt is made to send an `RST_STREAM` frame to the connected peer if
+one hasn't already been sent.
 
-When the `Http2Stream` instance is destroyed, the `'close'` event will
-be emitted. Because `Http2Stream` is an instance of `stream.Duplex`, the
-`'end'` event will also be emitted if the stream data is currently flowing.
-The `'error'` event may also be emitted if `http2stream.destroy()` was called
-with an `Error` passed as the first argument.
+`'close'` is always emitted on destroy. `'end'` and `'finish'` fire if
+their respective halves completed before destroy. `'error'` fires when
+the destroy carries an error — either via `http2stream.destroy(err)`,
+or when the peer reset the stream before sending `END_STREAM`.
 
 After the `Http2Stream` has been destroyed, the `http2stream.destroyed`
 property will be `true` and the `http2stream.rstCode` property will specify the
@@ -1264,14 +1265,18 @@ destroyed.
 
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/63249
+    description: Documentation-only deprecation.
 -->
 
-The `'aborted'` event is emitted whenever a `Http2Stream` instance is
-abnormally aborted in mid-communication.
-Its listener does not expect any arguments.
+> Stability: 0 - Deprecated. Use `'close'` and `'error'` plus
+> `stream.destroyed`.
 
-The `'aborted'` event will only be emitted if the `Http2Stream` writable side
-has not been ended.
+Emitted when an `Http2Stream` is closed before the writable side has
+been ended (via `.end()` or auto-ended via `respond({ endStream: true })`).
+Listeners receive no arguments.
 
 #### Event: `'close'`
 
@@ -1283,19 +1288,29 @@ The `'close'` event is emitted when the `Http2Stream` is destroyed. Once
 this event is emitted, the `Http2Stream` instance is no longer usable.
 
 The HTTP/2 error code used when closing the stream can be retrieved using
-the `http2stream.rstCode` property. If the code is any value other than
-`NGHTTP2_NO_ERROR` (`0`), an `'error'` event will have also been emitted.
+the `http2stream.rstCode` property.
 
 #### Event: `'error'`
 
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/63249
+    description: >-
+      Also emitted on peer-initiated resets that arrive before
+      `END_STREAM` (`ERR_HTTP2_STREAM_ABORTED` for clean codes,
+      `ERR_HTTP2_STREAM_ERROR` otherwise). Locally-initiated resets
+      without an explicit error remain silent.
 -->
 
 * `error` {Error}
 
-The `'error'` event is emitted when an error occurs during the processing of
-an `Http2Stream`.
+Emitted when an error occurs processing the `Http2Stream`. This includes
+peer-initiated resets that arrive before the readable side has been
+fully delivered: a clean reset code (`NGHTTP2_NO_ERROR` or
+`NGHTTP2_CANCEL`) surfaces as [`ERR_HTTP2_STREAM_ABORTED`][], any other
+code as [`ERR_HTTP2_STREAM_ERROR`][].
 
 #### Event: `'frameError'`
 
@@ -1378,8 +1393,8 @@ added: v8.4.0
 
 * Type: {boolean}
 
-Set to `true` if the `Http2Stream` instance was aborted abnormally. When set,
-the `'aborted'` event will have been emitted.
+`true` if the `Http2Stream` was closed while the writable side was
+still open. When set, the `'aborted'` event was emitted.
 
 #### `http2stream.bufferSize`
 
@@ -1723,6 +1738,13 @@ stream.on('push', (headers, flags) => {
 
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/63249
+    description: >-
+      If no `'response'` listener is attached when the response headers
+      arrive, the response body is now silently discarded - matching
+      the `lib/http` client behaviour.
 -->
 
 * `headers` {HTTP/2 Headers Object}
@@ -1744,6 +1766,16 @@ req.on('response', (headers, flags) => {
 });
 ```
 
+If no `'response'` listener is attached at the moment the response
+arrives, the response body will be entirely discarded (the stream is
+silently resumed). However, if a `'response'` listener is added, the
+data from the response object **must** be consumed — either by calling
+`response.read()` whenever there is a `'readable'` event, by adding a
+`'data'` handler, or by calling the `.resume()` method. Until the data
+is consumed, the `'end'` event will not fire. Also, until the data is
+read, it will consume memory that can eventually lead to a "process
+out of memory" error.
+
 ```cjs
 const http2 = require('node:http2');
 const client = http2.connect('https://localhost');
@@ -4035,11 +4067,8 @@ data.
 added: v8.4.0
 -->
 
-The `'aborted'` event is emitted whenever a `Http2ServerRequest` instance is
-abnormally aborted in mid-communication.
-
-The `'aborted'` event will only be emitted if the `Http2ServerRequest` writable
-side has not been ended.
+The `'aborted'` event is emitted whenever a `Http2ServerRequest` instance
+is closed while the underlying writable side is still open.
 
 #### Event: `'close'`
 
@@ -5050,6 +5079,8 @@ you need to implement any fall-back behavior yourself.
 [`'unknownProtocol'`]: #event-unknownprotocol
 [`ClientHttp2Stream`]: #class-clienthttp2stream
 [`Duplex`]: stream.md#class-streamduplex
+[`ERR_HTTP2_STREAM_ABORTED`]: errors.md#err_http2_stream_aborted
+[`ERR_HTTP2_STREAM_ERROR`]: errors.md#err_http2_stream_error
 [`Http2ServerRequest`]: #class-http2http2serverrequest
 [`Http2ServerResponse`]: #class-http2http2serverresponse
 [`Http2Session` and Sockets]: #http2session-and-sockets

lib/internal/errors.js

@@ -1321,6 +1321,8 @@ E('ERR_HTTP2_SOCKET_UNBOUND',
 E('ERR_HTTP2_STATUS_101',
   'HTTP status code 101 (Switching Protocols) is forbidden in HTTP/2', Error);
 E('ERR_HTTP2_STATUS_INVALID', 'Invalid status code: %s', RangeError);
+E('ERR_HTTP2_STREAM_ABORTED',
+  'The stream was reset before the readable side was fully consumed', Error);
 E('ERR_HTTP2_STREAM_CANCEL', function(error) {
   let msg = 'The pending stream has been canceled';
   if (error) {

lib/internal/http2/compat.js

@@ -158,11 +158,11 @@ function onStreamEnd() {
 }
 
 function onStreamError(error) {
-  // This is purposefully left blank
-  //
-  // errors in compatibility mode are
-  // not forwarded to the request
-  // and response objects.
+  // Mirror HTTP/1's IncomingMessage._destroy: forward 'error' to req
+  // only when a listener is attached.
+  const request = this[kRequest];
+  if (request !== undefined && request.listenerCount('error') > 0)
+    request.emit('error', error);
 }
 
 function onRequestPause() {
@@ -460,7 +460,9 @@ function onStreamCloseResponse() {
   this.removeListener('wantTrailers', onStreamTrailersReady);
   this[kResponse] = undefined;
 
-  res.emit('finish');
+  // Only emit 'finish' when the underlying writable actually finished
+  if (this.writableFinished)
+    res.emit('finish');
   res.emit('close');
 }