diff options
author | Michael Lehenbauer <mikelehen@google.com> | 2019-11-27 19:11:56 +0300 |
---|---|---|
committer | Anna Henningsen <anna@addaleax.net> | 2019-11-30 21:37:42 +0300 |
commit | 6f313f9ab0944beba57e97a208ca10726845343d (patch) | |
tree | 0c5c6643d890f7f2c295b896adb6632cac2eb00f | |
parent | be84ceefb8f08c5cc4a8dc85275ca57d0b355a0d (diff) |
http2: fix session memory accounting after pausing
The ability to pause input processing was added in 8a4a193 but
introduced a session memory accounting mismatch leading to potential
NGHTTP2_ENHANCE_YOUR_CALM errors.
After pausing
(https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L871),
the early return on line 873 skips the
DecrementCurrentSessionMemory(stream_buf_.len) call below (line 878).
When we later finished processing the input chunk
(https://github.com/nodejs/node/blob/f36331c1bfa4c4c202346b05dc3bd672f653e4df/src/node_http2.cc#L1858),
we were calling DecrementCurrentSessionMemory(stream_buf_offset_) [line
1875] which was a no-op since we just set stream_buf_offset_ to 0 [line
1873].
The correct amount to decrement by is still stream_buf_.len, since
that's the amount we skipped previously (line 878).
Fixes: https://github.com/nodejs/node/issues/29223
Refs: https://github.com/nodejs/node/commit/164ac5b241b96089e6bad5bb83ea416966b3245f
PR-URL: https://github.com/nodejs/node/pull/30684
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
-rw-r--r-- | src/node_http2.cc | 5 | ||||
-rw-r--r-- | test/parallel/test-http2-large-writes-session-memory-leak.js | 55 |
2 files changed, 59 insertions, 1 deletions
diff --git a/src/node_http2.cc b/src/node_http2.cc index 17a8a859d91..9421c36f3be 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1879,7 +1879,10 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { nread = buf.size(); stream_buf_offset_ = 0; stream_buf_ab_.Reset(); - DecrementCurrentSessionMemory(stream_buf_offset_); + + // We have now fully processed the stream_buf_ input chunk (by moving the + // remaining part into buf, which will be accounted for below). + DecrementCurrentSessionMemory(stream_buf_.len); } // Shrink to the actual amount of used data. diff --git a/test/parallel/test-http2-large-writes-session-memory-leak.js b/test/parallel/test-http2-large-writes-session-memory-leak.js new file mode 100644 index 00000000000..641923c06c9 --- /dev/null +++ b/test/parallel/test-http2-large-writes-session-memory-leak.js @@ -0,0 +1,55 @@ +'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/29223. +// There was a "leak" in the accounting of session memory leading +// to streams eventually failing with NGHTTP2_ENHANCE_YOUR_CALM. + +const server = http2.createSecureServer({ + key: fixtures.readKey('agent2-key.pem'), + cert: fixtures.readKey('agent2-cert.pem'), +}); + +// Simple server that sends 200k and closes the stream. +const data200k = 'a'.repeat(200 * 1024); +server.on('stream', (stream) => { + stream.write(data200k); + stream.end(); +}); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`https://localhost:${server.address().port}`, { + ca: fixtures.readKey('agent2-cert.pem'), + servername: 'agent2', + + // Set maxSessionMemory to 1MB so the leak causes errors faster. + maxSessionMemory: 1 + }); + + // Repeatedly create a new stream and read the incoming data. Even though we + // only have one stream active at a time, prior to the fix for #29223, + // session memory would steadily increase and we'd eventually hit the 1MB + // maxSessionMemory limit and get NGHTTP2_ENHANCE_YOUR_CALM errors trying to + // create new streams. + let streamsLeft = 50; + function newStream() { + const stream = client.request({ ':path': '/' }); + + stream.on('data', () => { }); + + stream.on('close', () => { + if (streamsLeft-- > 0) { + newStream(); + } else { + client.destroy(); + server.close(); + } + }); + } + + newStream(); +})); |