Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: fix memory leak caused by premature listener removing #55966

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
79 changes: 79 additions & 0 deletions test/parallel/test-h2leak-destroy-session-on-socket-ended.js
Original file line number Diff line number Diff line change
@@ -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);

Check failure on line 25 in test/parallel/test-h2leak-destroy-session-on-socket-ended.js

View workflow job for this annotation

GitHub Actions / test-linux

--- stderr --- node:internal/assert/utils:281 throw err; ^ AssertionError [ERR_ASSERTION]: h2 session leaked, rss:1135849472 at Timeout.check [as _onTimeout] (/home/runner/work/node/node/test/parallel/test-h2leak-destroy-session-on-socket-ended.js:25:3) at listOnTimeout (node:internal/timers:614:17) at process.processTimers (node:internal/timers:549:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: false, expected: true, operator: '==' } Node.js v24.0.0-pre --- stdout --- secureConnection stream... false Draining setImmediate after writing Command: out/Release/node --expose-gc --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /home/runner/work/node/node/test/parallel/test-h2leak-destroy-session-on-socket-ended.js

Check failure on line 25 in test/parallel/test-h2leak-destroy-session-on-socket-ended.js

View workflow job for this annotation

GitHub Actions / test-macOS

--- stderr --- node:internal/assert/utils:281 throw err; ^ AssertionError [ERR_ASSERTION]: h2 session leaked, rss:1130397696 at Timeout.check [as _onTimeout] (/Users/runner/work/node/node/test/parallel/test-h2leak-destroy-session-on-socket-ended.js:25:3) at listOnTimeout (node:internal/timers:614:17) at process.processTimers (node:internal/timers:549:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: false, expected: true, operator: '==' } Node.js v24.0.0-pre --- stdout --- secureConnection stream... false Draining setImmediate after writing Command: out/Release/node --expose-gc --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /Users/runner/work/node/node/test/parallel/test-h2leak-destroy-session-on-socket-ended.js

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);
});
}
Loading