From 4a8907d72b5906a745d96bb784305879770baf3d Mon Sep 17 00:00:00 2001 From: rogertyang Date: Sat, 23 Nov 2024 17:00:54 +0800 Subject: [PATCH 1/2] http2: fix memory leak caused by premature listener removing Http2Session should always call ondone into JS to detach the handle. In some case, ondone is defered to be called by the StreamListener through WriteWrap, we should be careful of this before getting rid of the StreamListener. --- src/node_http2.cc | 4 +- ...-h2leak-destroy-session-on-socket-ended.js | 79 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-h2leak-destroy-session-on-socket-ended.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 593c8b5f07a2a4..6094d7f9a8328e 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -803,13 +803,15 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { CHECK_EQ(nghttp2_session_terminate_session(session_.get(), code), 0); SendPendingData(); } else if (stream_ != nullptr) { + // so that the previous listener of the socket, typically, JS code of a (tls) socket + // will be notified of any activity later stream_->RemoveStreamListener(this); } set_destroyed(); // If we are writing we will get to make the callback in OnStreamAfterWrite. - if (!is_write_in_progress()) { + if (!is_write_in_progress() || !stream_) { Debug(this, "make done session callback"); HandleScope scope(env()->isolate()); MakeCallback(env()->ondone_string(), 0, nullptr); diff --git a/test/parallel/test-h2leak-destroy-session-on-socket-ended.js b/test/parallel/test-h2leak-destroy-session-on-socket-ended.js new file mode 100644 index 00000000000000..ca4a8e2c891c98 --- /dev/null +++ b/test/parallel/test-h2leak-destroy-session-on-socket-ended.js @@ -0,0 +1,79 @@ +'use strict'; +// Flags: --expose-gc + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); + +const server = http2.createSecureServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), +}); + +let connected; +let firstServerStream; + +const i = setInterval(function check() { + if (!connected || firstServerStream) return; + + global.gc(); + const rss = process.memoryUsage().rss; + assert((rss / 1024 / 1024 / 1024) < 1, 'h2 session leaked, rss:' + rss); + + clearInterval(i); + server.close(); +}, 1000); + + +server.on('secureConnection', (s) => { + console.log('secureConnection'); + s.on('end', () => { + console.log(s.destroyed); // false !! + s.destroy(); + firstServerStream.session.destroy(); + + firstServerStream = null; + }); +}); + +server.on('stream', (stream) => { + console.log('stream...'); + stream.session.memoryHolder = Buffer.alloc(1024 * 1024 * 1024, 1); + stream.write('a'.repeat(1024)); + firstServerStream = stream; + connected = true; + setImmediate(() => console.log('Draining setImmediate after writing')); +}); + + +server.listen(() => { + client(); +}); + + +const h2fstStream = [ + 'UFJJICogSFRUUC8yLjANCg0KU00NCg0K', + // http message (1st stream:) + 'AAAABAAAAAAA', + 'AAAPAQUAAAABhIJBiqDkHROdCbjwHgeG', +]; +function client() { + const client = tls.connect({ + port: server.address().port, + host: 'localhost', + rejectUnauthorized: false, + ALPNProtocols: ['h2'] + }, () => { + client.end(Buffer.concat(h2fstStream.map((s) => Buffer.from(s, 'base64'))), (err) => { + assert.ifError(err); + }); + }); + + client.on('error', (error) => { + console.error('Connection error:', error); + }); +} From f9f55cf02cda6f203205fe06e768e69eafca667c Mon Sep 17 00:00:00 2001 From: rogertyang Date: Sat, 23 Nov 2024 17:19:49 +0800 Subject: [PATCH 2/2] http2: fix memory leak caused by premature listener removing fix lint --- src/node_http2.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 6094d7f9a8328e..888f70bd4df8a3 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -803,8 +803,8 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { CHECK_EQ(nghttp2_session_terminate_session(session_.get(), code), 0); SendPendingData(); } else if (stream_ != nullptr) { - // so that the previous listener of the socket, typically, JS code of a (tls) socket - // will be notified of any activity later + // so that the previous listener of the socket, typically, JS code of a + // (tls) socket will be notified of any activity later stream_->RemoveStreamListener(this); }