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 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]>
nodejs · Sep 20, 2019 · d6ba106 · d6ba106
1 parent 4374d28
commit d6ba106
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 1 deletion.
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -745,8 +745,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
@@ -1225,6 +1227,7 @@ 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;
   }
@@ -1583,6 +1586,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
   ClearOutgoing(status);
 
   if ((flags_ & SESSION_STATE_READING_STOPPED) &&
+      !(flags_ & SESSION_STATE_WRITE_IN_PROGRESS) &&
       nghttp2_session_want_read(session_)) {
     flags_ &= ~SESSION_STATE_READING_STOPPED;
     stream_->ReadStart();

diff --git a/test/parallel/test-http2-multistream-destroy-on-read-tls.js b/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();
+      }
+    }));
+  }
+}));