Skip to content

http2: add session tracking and graceful server shutdown of http2 server #57586

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.

Sign up for GitHub

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

Closed
pandeykushagra51 wants to merge 7 commits into from
Closed

http2: add session tracking and graceful server shutdown of http2 server #57586

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb71a11
http2: session tracking and graceful server close
pandeykushagra51 Mar 21, 2025
7a852db
fix: improve HTTP/2 server shutdown to prevent race conditions
pandeykushagra51 Mar 28, 2025
6bc96f1
http2: fix cross-platform test timing issues
pandeykushagra51 Mar 29, 2025
dd751ae
resolved pr comments on test-http2-graceful-close.js
pandeykushagra51 Apr 6, 2025
be55b6d
resolved pr comments on test-http2-server-close-idle-connection.js an…
pandeykushagra51 Apr 6, 2025
8ee2e3e
resolved pr comments: done suggested changes from lpinca
pandeykushagra51 Apr 7, 2025
90acf08
resolved pr comment: updated for each loop to for...of loop
pandeykushagra51 Apr 9, 2025
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
Prev Previous commit
Next Next commit
resolved pr comments on test-http2-server-close-idle-connection.js an… 
…d strengthen test
  • Loading branch information
@pandeykushagra51
pandeykushagra51 committed Apr 6, 2025
commit be55b6d47bd8d90f267025cc558714aefb252ad2
74 changes: 17 additions & 57 deletions test/parallel/test-http2-server-close-idle-connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,66 +8,30 @@ const http2 = require('http2');

// This test verifies that the server closes idle connections

// Track test state with flags
const states = {
serverListening: false,
initialRequestCompleted: false,
connectionBecameIdle: false,
serverClosedIdleConnection: false
};

const server = http2.createServer();

// Track server events
server.on('stream', common.mustCall((stream, headers) => {
assert.strictEqual(states.serverListening, true);

// Respond to the request with a small payload
stream.respond({
'content-type': 'text/plain',
':status': 200
});
stream.end('hello');

// After the request is done, the connection should become idle
stream.on('close', () => {
states.initialRequestCompleted = true;
// Mark connection as idle after the request completes
states.connectionBecameIdle = true;
});
}));

// Track session closure events
server.on('session', common.mustCall((session) => {
session.on('stream', common.mustCall((stream) => {
// Respond to the request with a small payload
stream.respond({
'content-type': 'text/plain',
':status': 200
});
stream.end('hello');
stream.on('close', common.mustCall(() => {
// This should only happen after the connection became idle
assert.strictEqual(states.connectionBecameIdle, true);
states.serverClosedIdleConnection = true;

// Test is successful, close the server
server.close(common.mustCall(() => {
console.log('server closed');
}));
assert.strictEqual(stream.writableFinished, true);
}));
Comment on lines +21 to +23
Copy link
Copy Markdown
Member

@lpinca lpinca Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stream.on('close', common.mustCall(() => {
assert.strictEqual(stream.writableFinished, true);
}));

Feel free to keep it, but I don't think it is useful as the server is closed after the request ends (the 'end' event is emitted on the client).

Copy link
Copy Markdown
Contributor Author

@pandeykushagra51 pandeykushagra51 Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a but different opinion, I would like to keep it

Copy link
Copy Markdown
Contributor Author

@pandeykushagra51 pandeykushagra51 Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should ensure that server side stream have closed before server close

Copy link
Copy Markdown
Member

@lpinca lpinca Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server does not close if there are open streams, no?

Copy link
Copy Markdown
Contributor Author

@pandeykushagra51 pandeykushagra51 Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO if there are open stream we can’t call it graceful closure

Copy link
Copy Markdown
Member

@lpinca lpinca Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the point, if the stream is not closed via server.close(), the process does not exit. If the process exits then it means that the stream and the server were closed. stream.writableFinished is already tested on the client side. Anyway, it does not matter.

Copy link
Copy Markdown
Contributor Author

@pandeykushagra51 pandeykushagra51 Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should still remove that?

}));
session.on('close', common.mustCall(() => {
console.log('session closed');
}));
session.on('close', common.mustCall());
}));

// Start the server
server.listen(0, common.mustCall(() => {
states.serverListening = true;

// Create client and initial request
const client = http2.connect(`http://localhost:${server.address().port}`);

// Track client session events
client.on('close', common.mustCall(() => {
// Verify server closed the connection after it became idle
assert.strictEqual(states.serverClosedIdleConnection, true);
}));
// This will ensure that server closed the idle connection
client.on('close', common.mustCall());

// Make an initial request
const req = client.request({ ':path': '/' });
Expand All @@ -84,17 +48,13 @@ server.listen(0, common.mustCall(() => {
// When initial request ends
req.on('end', common.mustCall(() => {
assert.strictEqual(data, 'hello');

// Don't explicitly close the client - keep it idle
// Wait for the server to close the idle connection
// Close the server as client is idle now
setImmediate(() => {
server.close(common.mustCall());
Copy link
Copy Markdown
Member

@lpinca lpinca Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
server.close(common.mustCall());
server.close();

Copy link
Copy Markdown
Contributor Author

@pandeykushagra51 pandeykushagra51 Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

});
}));

client.on('close', common.mustCall());

req.end();
// Don't explicitly close the client, we want to keep it
// idle and check if the server closes the idle connection.
// As this is what we want to test here.
}));

// Final verification on exit
process.on('exit', () => {
assert.strictEqual(states.serverClosedIdleConnection, true);
});
Loading