Skip to content
Permalink
Browse files

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 <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information...
addaleax authored and BethGriggs committed Sep 1, 2019
1 parent 559a8e3 commit e45b6a3b98b95a16cd8e34914e64c40fe51af8e9
Showing with 52 additions and 1 deletion.
  1. +5 −1 src/node_http2.cc
  2. +47 −0 test/parallel/test-http2-multistream-destroy-on-read-tls.js
@@ -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();
@@ -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();
}
}));
}
}));

0 comments on commit e45b6a3

Please sign in to comment.
You can’t perform that action at this time.