Skip to content

Commit d16b0ba

Browse files
pandeykushagra51RafaelGSS
authored andcommitted
http2: session tracking and graceful server close
This change adds proper tracking of HTTP / 2 server sessions to ensure they are gracefully closed when the server is shut down.It implements: - A new kSessions symbol for tracking active sessions - Adding/removing sessions from a SafeSet in the server - A closeAllSessions helper function to close active sessions - Updates to Http2Server and Http2SecureServer close methods Breaking Change: any client trying to create new requests on existing connections will not be able to do so once server close is initiated Refs: https://datatracker.ietf.org/doc/html/rfc7540\#section-9.1 Refs: https://nodejs.org/api/http.html\#serverclosecallback - improve HTTP/2 server shutdown to prevent race conditions 1. Fix server shutdown race condition - Stop listening for new connections before closing existing ones - Ensure server.close() properly completes in all scenarios 2. Improve HTTP/2 tests - Replace setTimeout with event-based flow control - Simplify test logic for better readability - Add clear state tracking for event ordering - Improve assertions to verify correct shutdown sequence This eliminates a race condition where new sessions could connect between the time existing sessions are closed and the server stops listening, potentially preventing the server from fully shutting down. - fix cross-platform test timing issues Fix test-http2-server-http1-client.js failure on Ubuntu by deferring server.close() to next event loop cycle. The issue only affected Ubuntu where session close occurs before error emission, causing the test to miss errors when HTTP/1 clients connect to HTTP/2 servers. Using setImmediate() ensures error events fire before server close across all platforms while maintaining recent session handling improvements. PR-URL: #57586 Fixes: #57611 Refs: https://datatracker.ietf.org/doc/html/rfc7540#section-9.1 Refs: https://nodejs.org/api/http.html#serverclosecallback Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent cdedec7 commit d16b0ba

7 files changed

+164
-6
lines changed

lib/internal/http2/core.js

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ const kServer = Symbol('server');
251251
const kState = Symbol('state');
252252
const kType = Symbol('type');
253253
const kWriteGeneric = Symbol('write-generic');
254+
const kSessions = Symbol('sessions');
254255

255256
const {
256257
kBitfield,
@@ -1126,9 +1127,13 @@ function emitClose(self, error) {
11261127
function cleanupSession(session) {
11271128
const socket = session[kSocket];
11281129
const handle = session[kHandle];
1130+
const server = session[kServer];
11291131
session[kProxySocket] = undefined;
11301132
session[kSocket] = undefined;
11311133
session[kHandle] = undefined;
1134+
if (server) {
1135+
server[kSessions].delete(session);
1136+
}
11321137
session[kNativeFields] = trackAssignmentsTypedArray(
11331138
new Uint8Array(kSessionUint8FieldCount));
11341139
if (handle)
@@ -1651,6 +1656,9 @@ class ServerHttp2Session extends Http2Session {
16511656
constructor(options, socket, server) {
16521657
super(NGHTTP2_SESSION_SERVER, options, socket);
16531658
this[kServer] = server;
1659+
if (server) {
1660+
server[kSessions].add(this);
1661+
}
16541662
// This is a bit inaccurate because it does not reflect changes to
16551663
// number of listeners made after the session was created. This should
16561664
// not be an issue in practice. Additionally, the 'priority' event on
@@ -3175,11 +3183,25 @@ function onErrorSecureServerSession(err, socket) {
31753183
socket.destroy(err);
31763184
}
31773185

3186+
/**
3187+
* This function closes all active sessions gracefully.
3188+
* @param {*} server the underlying server whose sessions to be closed
3189+
*/
3190+
function closeAllSessions(server) {
3191+
const sessions = server[kSessions];
3192+
if (sessions.size > 0) {
3193+
for (const session of sessions) {
3194+
session.close();
3195+
}
3196+
}
3197+
}
3198+
31783199
class Http2SecureServer extends TLSServer {
31793200
constructor(options, requestListener) {
31803201
options = initializeTLSOptions(options);
31813202
super(options, connectionListener);
31823203
this[kOptions] = options;
3204+
this[kSessions] = new SafeSet();
31833205
this.timeout = 0;
31843206
this.on('newListener', setupCompat);
31853207
if (options.allowHTTP1 === true) {
@@ -3209,10 +3231,11 @@ class Http2SecureServer extends TLSServer {
32093231
}
32103232

32113233
close() {
3234+
ReflectApply(TLSServer.prototype.close, this, arguments);
32123235
if (this[kOptions].allowHTTP1 === true) {
32133236
httpServerPreClose(this);
32143237
}
3215-
ReflectApply(TLSServer.prototype.close, this, arguments);
3238+
closeAllSessions(this);
32163239
}
32173240

32183241
closeIdleConnections() {
@@ -3227,6 +3250,7 @@ class Http2Server extends NETServer {
32273250
options = initializeOptions(options);
32283251
super(options, connectionListener);
32293252
this[kOptions] = options;
3253+
this[kSessions] = new SafeSet();
32303254
this.timeout = 0;
32313255
this.on('newListener', setupCompat);
32323256
if (typeof requestListener === 'function')
@@ -3248,6 +3272,11 @@ class Http2Server extends NETServer {
32483272
this[kOptions].settings = { ...this[kOptions].settings, ...settings };
32493273
}
32503274

3275+
close() {
3276+
ReflectApply(NETServer.prototype.close, this, arguments);
3277+
closeAllSessions(this);
3278+
}
3279+
32513280
async [SymbolAsyncDispose]() {
32523281
return promisify(super.close).call(this);
32533282
}

test/parallel/test-http2-capture-rejection.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,6 @@ events.captureRejections = true;
108108
server.on('stream', common.mustCall(async (stream) => {
109109
const { port } = server.address();
110110

111-
server.close();
112-
113111
stream.pushStream({
114112
':scheme': 'http',
115113
':path': '/foobar',
@@ -127,6 +125,8 @@ events.captureRejections = true;
127125
stream.respond({
128126
':status': 200
129127
});
128+
129+
server.close();
130130
}));
131131

132132
server.listen(0, common.mustCall(() => {

test/parallel/test-http2-compat-serverresponse-statusmessage-property-set.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ server.listen(0, common.mustCall(function() {
2424
response.statusMessage = 'test';
2525
response.statusMessage = 'test'; // only warn once
2626
assert.strictEqual(response.statusMessage, ''); // no change
27-
server.close();
2827
}));
2928
response.end();
3029
}));
@@ -44,6 +43,9 @@ server.listen(0, common.mustCall(function() {
4443
request.on('end', common.mustCall(function() {
4544
client.close();
4645
}));
46+
request.on('close', common.mustCall(function() {
47+
server.close();
48+
}));
4749
request.end();
4850
request.resume();
4951
}));

test/parallel/test-http2-compat-serverresponse-statusmessage-property.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ server.listen(0, common.mustCall(function() {
2323
response.on('finish', common.mustCall(function() {
2424
assert.strictEqual(response.statusMessage, '');
2525
assert.strictEqual(response.statusMessage, ''); // only warn once
26-
server.close();
2726
}));
2827
response.end();
2928
}));
@@ -43,6 +42,9 @@ server.listen(0, common.mustCall(function() {
4342
request.on('end', common.mustCall(function() {
4443
client.close();
4544
}));
45+
request.on('close', common.mustCall(function() {
46+
server.close();
47+
}));
4648
request.end();
4749
request.resume();
4850
}));
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
const assert = require('assert');
7+
const http2 = require('http2');
8+
9+
// This test verifies that the server waits for active connections/sessions
10+
// to complete and all data to be sent before fully closing
11+
12+
// Create a server that will send a large response in chunks
13+
const server = http2.createServer();
14+
15+
// Track server events
16+
server.on('stream', common.mustCall((stream, headers) => {
17+
18+
// Initiate the server close before client data is sent, this will
19+
// test if the server properly waits for the stream to finish
20+
server.close();
21+
setImmediate(() => {
22+
stream.respond({
23+
'content-type': 'text/plain',
24+
':status': 200
25+
});
26+
27+
// Create a large response (1MB) to ensure it takes some time to send
28+
const chunk = Buffer.alloc(64 * 1024, 'x');
29+
30+
// Send 16 chunks (1MB total) to simulate a large response
31+
for (let i = 0; i < 16; i++) {
32+
stream.write(chunk);
33+
}
34+
35+
// Stream end should happen after data is written
36+
stream.end();
37+
});
38+
39+
stream.on('close', common.mustCall(() => {
40+
assert.strictEqual(stream.readableEnded, true);
41+
assert.strictEqual(stream.writableFinished, true);
42+
}));
43+
}));
44+
45+
// Start the server
46+
server.listen(0, common.mustCall(() => {
47+
// Create client and request
48+
const client = http2.connect(`http://localhost:${server.address().port}`);
49+
const req = client.request({ ':path': '/' });
50+
51+
// Track received data
52+
let receivedData = 0;
53+
req.on('data', (chunk) => {
54+
receivedData += chunk.length;
55+
});
56+
57+
// When request closes
58+
req.on('close', common.mustCall(() => {
59+
// Should receive all data
60+
assert.strictEqual(req.readableEnded, true);
61+
assert.strictEqual(receivedData, 64 * 1024 * 16);
62+
assert.strictEqual(req.writableFinished, true);
63+
}));
64+
65+
// Start the request
66+
req.end();
67+
}));
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
const assert = require('assert');
7+
const http2 = require('http2');
8+
9+
// This test verifies that the server closes idle connections
10+
11+
const server = http2.createServer();
12+
13+
server.on('session', common.mustCall((session) => {
14+
session.on('stream', common.mustCall((stream) => {
15+
// Respond to the request with a small payload
16+
stream.respond({
17+
'content-type': 'text/plain',
18+
':status': 200
19+
});
20+
stream.end('hello');
21+
stream.on('close', common.mustCall(() => {
22+
assert.strictEqual(stream.writableFinished, true);
23+
}));
24+
}));
25+
session.on('close', common.mustCall());
26+
}));
27+
28+
// Start the server
29+
server.listen(0, common.mustCall(() => {
30+
// Create client and initial request
31+
const client = http2.connect(`http://localhost:${server.address().port}`);
32+
33+
// This will ensure that server closed the idle connection
34+
client.on('close', common.mustCall());
35+
36+
// Make an initial request
37+
const req = client.request({ ':path': '/' });
38+
39+
// Process the response data
40+
req.setEncoding('utf8');
41+
let data = '';
42+
req.on('data', (chunk) => {
43+
data += chunk;
44+
});
45+
46+
// When initial request ends
47+
req.on('end', common.mustCall(() => {
48+
assert.strictEqual(data, 'hello');
49+
// Close the server as client is idle now
50+
setImmediate(() => {
51+
server.close();
52+
});
53+
}));
54+
55+
// Don't explicitly close the client, we want to keep it
56+
// idle and check if the server closes the idle connection.
57+
// As this is what we want to test here.
58+
}));

test/parallel/test-http2-server-http1-client.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,5 +23,5 @@ server.on('session', common.mustCall((session) => {
2323

2424
server.listen(0, common.mustCall(() => {
2525
const req = http.get(`http://localhost:${server.address().port}`);
26-
req.on('error', (error) => server.close());
26+
req.on('error', (error) => setImmediate(() => server.close()));
2727
}));

0 commit comments

Comments
 (0)