http2: send deferred RST_STREAM on stream destroy

lib/internal/http2/core.js

@@ -348,6 +348,7 @@ const STREAM_FLAGS_HEADERS_SENT = 0x4;
 const STREAM_FLAGS_HEAD_REQUEST = 0x8;
 const STREAM_FLAGS_ABORTED = 0x10;
 const STREAM_FLAGS_HAS_TRAILERS = 0x20;
+const STREAM_FLAGS_RST_DEFERRED = 0x40;
 
 const SESSION_FLAGS_PENDING = 0x0;
 const SESSION_FLAGS_READY = 0x1;
@@ -644,6 +645,8 @@ function onStreamClose(code, peerReset) {
   stream[kState].fd = -1;
 
   if (locallyInitiated) {
+    // nghttp2 has already closed the stream, a deferred RST_STREAM is moot.
+    stream[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED;
     stream.destroy();
     return true;
   }
@@ -2166,6 +2169,9 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) {
         rstStreamStatus === kForceRstStream) {
       finishCloseStream.call(stream, code);
     } else {
+      // Remember that the RST_STREAM is still owed, so that destroying the
+      // stream before 'finish' can submit it. See Http2Stream#_destroy().
+      state.flags |= STREAM_FLAGS_RST_DEFERRED;
       stream.once('finish', finishCloseStream.bind(stream, code));
     }
   }
@@ -2188,6 +2194,7 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) {
 }
 
 function finishCloseStream(code) {
+  this[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED;
   const rstStreamFn = submitRstStream.bind(this, code);
   // Defer until the stream has an ID assigned.
   if (this.pending) {
@@ -2681,8 +2688,16 @@ class Http2Stream extends Duplex {
     }
     const hasHandle = handle !== undefined;
 
-    if (!this.closed)
+    if (!this.closed) {
       closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream);
+    } else if (state.flags & STREAM_FLAGS_RST_DEFERRED) {
+      // close() deferred the RST_STREAM until 'finish', which will never be
+      // emitted now. Submit it here, otherwise nghttp2 keeps the stream open
+      // with a data provider that no longer has a stream to read from.
+      state.flags &= ~STREAM_FLAGS_RST_DEFERRED;
+      if (hasHandle)
+        handle.rstStream(code);
+    }
     this.push(null);
 
 

src/node_http2.cc

@@ -2840,7 +2840,10 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle,
   Http2Session* session = static_cast<Http2Session*>(user_data);
   Debug(session, "reading outbound data for stream %d", id);
   BaseObjectPtr<Http2Stream> stream = session->FindStream(id);
-  if (!stream) return 0;
+  // The stream was destroyed without nghttp2 being told. Returning 0 here
+  // would make nghttp2 send empty DATA frames forever, so have it reset
+  // the stream instead.
+  if (!stream) return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
   if (stream->statistics_.first_byte_sent == 0)
     stream->statistics_.first_byte_sent = uv_hrtime();
   CHECK_EQ(id, stream->id());

test/parallel/test-http2-close-destroy-queued-body.js

@@ -0,0 +1,46 @@
+'use strict';
+
+// Destroying a stream that was closed with NO_ERROR while its request body
+// was still queued behind flow control must still send RST_STREAM.
+// Refs: https://github.com/nodejs/node/issues/67363
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+const { NGHTTP2_NO_ERROR } = http2.constants;
+
+// A small stream window makes sure that most of the body stays queued.
+const server = http2.createServer({ settings: { initialWindowSize: 16384 } });
+
+let req;
+
+server.on('stream', common.mustCall((stream) => {
+  // The stream is not being read yet, so the request body is stuck behind
+  // flow control when the client closes and destroys the request.
+  req.close();
+  req.destroy();
+
+  stream.on('error', common.expectsError({
+    code: 'ERR_HTTP2_STREAM_ABORTED',
+  }));
+  stream.on('close', common.mustCall(() => {
+    assert.strictEqual(stream.rstCode, NGHTTP2_NO_ERROR);
+    server.close();
+  }));
+  // Open the window again. Without the RST_STREAM this is what makes the
+  // client loop.
+  stream.resume();
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  req = client.request({ ':method': 'POST' });
+  req.on('error', common.mustNotCall());
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.rstCode, NGHTTP2_NO_ERROR);
+    client.close();
+  }));
+  req.end(Buffer.alloc(1024 * 1024));
+}));

test/parallel/test-http2-server-close-destroy-queued-body.js

@@ -0,0 +1,51 @@
+'use strict';
+
+// Destroying a stream that was closed with NO_ERROR while its response body
+// was still queued behind flow control must still send RST_STREAM.
+// Refs: https://github.com/nodejs/node/issues/67363
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+const { NGHTTP2_NO_ERROR } = http2.constants;
+
+const server = http2.createServer();
+
+let serverStream;
+
+server.on('stream', common.mustCall((stream) => {
+  serverStream = stream;
+  stream.on('error', common.mustNotCall());
+  stream.on('close', common.mustCall(() => {
+    assert.strictEqual(stream.rstCode, NGHTTP2_NO_ERROR);
+  }));
+  stream.respond();
+  stream.end(Buffer.alloc(1024 * 1024));
+}));
+
+server.listen(0, common.mustCall(() => {
+  // A small stream window makes sure that most of the body stays queued.
+  const client = http2.connect(`http://localhost:${server.address().port}`,
+                               { settings: { initialWindowSize: 16384 } });
+  const req = client.request();
+  req.on('response', common.mustCall(() => {
+    // The response is not being read yet, so the response body is stuck
+    // behind flow control when the server closes and destroys the stream.
+    serverStream.close();
+    serverStream.destroy();
+    // Open the window again. Without the RST_STREAM this is what makes the
+    // server loop.
+    req.resume();
+  }));
+  req.on('error', common.expectsError({
+    code: 'ERR_HTTP2_STREAM_ABORTED',
+  }));
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.rstCode, NGHTTP2_NO_ERROR);
+    client.close();
+    server.close();
+  }));
+  req.end();
+}));