From f6642cfeae4b143a679918ab0458f70b8d3cf30f Mon Sep 17 00:00:00 2001 From: David Halls Date: Thu, 27 Sep 2018 22:28:50 +0100 Subject: [PATCH 1/4] Forget closed http2 streams Fixes: https://github.com/nodejs/node/issues/23116 --- src/node_http2.cc | 2 + .../test-http2-forget-closed-streams.js | 58 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 test/parallel/test-http2-forget-closed-streams.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 9a0cc4ae191ed0..6ed4e6e2526fe5 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -98,6 +98,8 @@ Http2Scope::~Http2Scope() { Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { nghttp2_option_new(&options_); + nghttp2_option_set_no_closed_streams(options_, 1); + // We manually handle flow control within a session in order to // implement backpressure -- that is, we only send WINDOW_UPDATE // frames to the remote peer as data is actually consumed by user diff --git a/test/parallel/test-http2-forget-closed-streams.js b/test/parallel/test-http2-forget-closed-streams.js new file mode 100644 index 00000000000000..5cbd2b4a3ef197 --- /dev/null +++ b/test/parallel/test-http2-forget-closed-streams.js @@ -0,0 +1,58 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) { + common.skip('missing crypto'); +} + +// Issue #23116 +// nghttp2 keeps closed stream structures around in memory (couple of hundred +// bytes each) until a session is closed. It does this to maintain the priority +// tree. However, it limits the number of requests that can be made in a +// session before our memory tracking (correctly) kicks in. +// The fix is to tell nghttp2 to forget about closed streams. We don't make use +// of priority anyway. +// Without the fix, this test fails at ~40k requests with an exception: +// Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code +// NGHTTP2_ENHANCE_YOUR_CALM + +const http2 = require('http2'); +const assert = require('assert'); +const { Writable } = require('stream'); + +const server = http2.createServer(); + +server.on('session', function(session) { + session.on('stream', function(stream) { + stream.on('end', common.mustCall(function() { + this.respond({ + ':status': 200 + }, { + endStream: true + }); + })); + stream.pipe(new Writable({ + write(chunk, encoding, cb) { + cb(); + } + })); + }); +}); + +server.listen(0, function() { + const client = http2.connect(`http://localhost:${server.address().port}`); + + function next(i) { + if (i === 100000) { + client.close(); + return server.close(); + } + const stream = client.request({ ':method': 'POST' }); + stream.on('response', common.mustCall(function(headers) { + assert.strictEqual(headers[':status'], 200); + this.on('close', common.mustCall(() => next(i + 1))); + })); + stream.end(); + } + + next(0); +}); From 750701510b99f2f9efbeee6fa90bf882ac3a619f Mon Sep 17 00:00:00 2001 From: David Halls Date: Thu, 27 Sep 2018 22:38:38 +0100 Subject: [PATCH 2/4] Add comment explaining call to nghttp2_option_set_no_closed_streams --- src/node_http2.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/node_http2.cc b/src/node_http2.cc index 6ed4e6e2526fe5..09662b08a4f022 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -98,6 +98,8 @@ Http2Scope::~Http2Scope() { Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { nghttp2_option_new(&options_); + // Make sure closed connections aren't kept around, taking up memory. + // Note that this breaks the priority tree, which we don't use. nghttp2_option_set_no_closed_streams(options_, 1); // We manually handle flow control within a session in order to From 6467dc1dca19bc28ef8bd626354f13f35eaba402 Mon Sep 17 00:00:00 2001 From: David Halls Date: Fri, 28 Sep 2018 07:19:47 +0100 Subject: [PATCH 3/4] Issue #23116: Decrease test run time Decreasing maxSessionMemory lets us decrease number of iteractions before the exception occurs without the fix. --- test/parallel/test-http2-forget-closed-streams.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-http2-forget-closed-streams.js b/test/parallel/test-http2-forget-closed-streams.js index 5cbd2b4a3ef197..55003c6441f81d 100644 --- a/test/parallel/test-http2-forget-closed-streams.js +++ b/test/parallel/test-http2-forget-closed-streams.js @@ -19,7 +19,7 @@ const http2 = require('http2'); const assert = require('assert'); const { Writable } = require('stream'); -const server = http2.createServer(); +const server = http2.createServer({ maxSessionMemory: 1 }); server.on('session', function(session) { session.on('stream', function(stream) { @@ -42,7 +42,7 @@ server.listen(0, function() { const client = http2.connect(`http://localhost:${server.address().port}`); function next(i) { - if (i === 100000) { + if (i === 10000) { client.close(); return server.close(); } From 4a7367f5bfac5e7ca1b865a22a69e1c94842b1d0 Mon Sep 17 00:00:00 2001 From: David Halls Date: Tue, 2 Oct 2018 20:00:57 +0100 Subject: [PATCH 4/4] Use resume to consume stream in test --- test/parallel/test-http2-forget-closed-streams.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/parallel/test-http2-forget-closed-streams.js b/test/parallel/test-http2-forget-closed-streams.js index 55003c6441f81d..c0b3bcd819140c 100644 --- a/test/parallel/test-http2-forget-closed-streams.js +++ b/test/parallel/test-http2-forget-closed-streams.js @@ -17,7 +17,6 @@ if (!common.hasCrypto) { const http2 = require('http2'); const assert = require('assert'); -const { Writable } = require('stream'); const server = http2.createServer({ maxSessionMemory: 1 }); @@ -30,11 +29,7 @@ server.on('session', function(session) { endStream: true }); })); - stream.pipe(new Writable({ - write(chunk, encoding, cb) { - cb(); - } - })); + stream.resume(); }); });