http2: do not start reading after write if new write is on wire

Don’t start reading more input data if we’re still busy writing output.
This was overlooked in 8a4a193.

Fixes: #29353
Fixes: #29393

PR-URL: #29399
Backport-PR-URL: #29618
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

Author: addaleax

src/node_http2.cc

@@ -791,8 +791,10 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
   flags_ |= SESSION_STATE_CLOSING;
 
   // Stop reading on the i/o stream
-  if (stream_ != nullptr)
+  if (stream_ != nullptr) {
+    flags_ |= SESSION_STATE_READING_STOPPED;
     stream_->ReadStop();
+  }
 
   // If the socket is not closed, then attempt to send a closing GOAWAY
   // frame. There is no guarantee that this GOAWAY will be received by
@@ -1280,6 +1282,7 @@ inline int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
   // If we are currently waiting for a write operation to finish, we should
   // tell nghttp2 that we want to wait before we process more input data.
   if (session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS) {
+    CHECK_NE(session->flags_ & SESSION_STATE_READING_STOPPED, 0);
     session->flags_ |= SESSION_STATE_NGHTTP2_RECV_PAUSED;
     return NGHTTP2_ERR_PAUSE;
   }
@@ -1626,6 +1629,7 @@ void Http2Session::OnStreamAfterWriteImpl(WriteWrap* w, int status, void* ctx) {
   session->ClearOutgoing(status);
 
   if ((session->flags_ & SESSION_STATE_READING_STOPPED) &&
+      !(session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS) &&
       nghttp2_session_want_read(session->session_)) {
     session->flags_ &= ~SESSION_STATE_READING_STOPPED;
     session->stream_->ReadStart();

test/parallel/test-http2-multistream-destroy-on-read-tls.js

@@ -0,0 +1,47 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const fixtures = require('../common/fixtures');
+const http2 = require('http2');
+
+// Regression test for https://github.com/nodejs/node/issues/29353.
+// Test that it’s okay for an HTTP2 + TLS server to destroy a stream instance
+// while reading it.
+
+const server = http2.createSecureServer({
+  key: fixtures.readKey('agent2-key.pem'),
+  cert: fixtures.readKey('agent2-cert.pem')
+});
+
+const filenames = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'];
+
+server.on('stream', common.mustCall((stream) => {
+  function write() {
+    stream.write('a'.repeat(10240));
+    stream.once('drain', write);
+  }
+  write();
+}, filenames.length));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`https://localhost:${server.address().port}`, {
+    ca: fixtures.readKey('agent2-cert.pem'),
+    servername: 'agent2'
+  });
+
+  let destroyed = 0;
+  for (const entry of filenames) {
+    const stream = client.request({
+      ':path': `/${entry}`
+    });
+    stream.once('data', common.mustCall(() => {
+      stream.destroy();
+
+      if (++destroyed === filenames.length) {
+        client.destroy();
+        server.close();
+      }
+    }));
+  }
+}));