-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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: add session tracking and graceful server shutdown of http2 server #57586
base: main
Are you sure you want to change the base?
http2: add session tracking and graceful server shutdown of http2 server #57586
Conversation
Review requested:
|
This strategy is adopted in other server like Springboot (Tomcat) and golang as well for gracefull server close |
Please review the changes, I will be very happy to work on any suggestion and any scope of improvement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57586 +/- ##
==========================================
- Coverage 90.23% 90.21% -0.02%
==========================================
Files 630 630
Lines 185055 185547 +492
Branches 36221 36392 +171
==========================================
+ Hits 166984 167397 +413
+ Misses 11043 11030 -13
- Partials 7028 7120 +92
🚀 New features to boost your workflow:
|
I just took a quick look, but can't all of this be done in userland? What is the advantage of doing this in core and forcing session tracking for everyone? |
No doubt that this can be done on user side as well but this feature is surely going to provide convenience to users. Line 1800 in 0a91e98
Line 519 in 0a91e98
Also this strategy is widely adopted in other server like go and springboot (tomacat). |
It will be really helpful if someone can tell what could be possible reason for test failure? I have tried running them locally (on mac-os) and all test test passed as expected. |
Also I have seen an issue #55459 which is under triaged state, that can also be done easily once this PR is merged. |
update regarding failing test cases:
|
That's not how it works in Node.js. The The Changing the As for testing, if possible, do not use timers as they make the tests flaky. |
In my opinion, any session which are not having any open request/stream will be referred as idle session. Please let me know your thoughts on this.
Confirming the behavior again, in current changes, active sessions are not closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @pandeykushagra51!
HTTP servers server.close()
nowadays does close idle connections, and personally I'd be happy for HTTP/2 servers to match that behaviour, although as a breaking change. The basic approach seems sensible to me (although note the race condition I've commented on here).
AFAICT it does seem like this PR does actually match that close-idle behaviour already (it calls session.close()
, which apparently allows all pending streams to complete before closing the session) so this isn't unreasonable. That said, we do definitely need a test which calls server.close()
while a request is still pending and then checks it completes OK and closes afterwards.
The current tests here need some substantial changes though I think. Most importantly: we shouldn't be using setTimeout and timing checks anywhere in here. That will make these tests very flaky (as shown by CI here) and much slower than they need to be. There's also quite complicated, it seems like they're testing a few different things and it's not exactly clear what's going on.
We should avoid depending on specific timing (by instead waiting for events, and then triggering the next step after each previous event, etc), and these tests should be simplified and/or broken into separate tests too I think. With those changes, that will probably fix the CI test failures here. You might find it useful to take a look at the HTTP & net server close tests for some examples.
lib/internal/http2/core.js
Outdated
@@ -3205,6 +3227,7 @@ class Http2SecureServer extends TLSServer { | |||
if (this[kOptions].allowHTTP1 === true) { | |||
httpServerPreClose(this); | |||
} | |||
closeAllSessions(this); | |||
ReflectApply(TLSServer.prototype.close, this, arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same race condition as below. We might also need to do this before httpServerPreClose
as well (needs investigation) and we should check this works for any HTTP/1 connections here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pimterry could you please review again
thanks a lot @pimterry for your input, I will b working on these and will update the PR as requested |
It makes sense. |
@pandeykushagra51 can you please rebase this against main, apply the new changes and force push here instead? In this way we have all the context/comments in the same place. |
f2a480c
to
da31e96
Compare
done the changes, thanks for informing 🙂 |
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
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.
// When the request completes | ||
req.on('end', common.mustCall()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// When the request completes | |
req.on('end', common.mustCall()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// Should receive all data | ||
assert.strictEqual(receivedData, 64 * 1024 * 16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Should receive all data | |
assert.strictEqual(receivedData, 64 * 1024 * 16); | |
// Should receive all data | |
assert.strictEqual(req.readableEnded, true); | |
assert.strictEqual(receivedData, 64 * 1024 * 16); | |
assert.strictEqual(req.writableFinished, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}); | ||
|
||
stream.on('close', common.mustCall(() => { | ||
assert.strictEqual(stream.writableFinished, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.strictEqual(stream.writableFinished, true); | |
assert.strictEqual(stream.readableFinished, true); | |
assert.strictEqual(stream.writableFinished, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just found from nodejs docs that readable do not have readableFinished
property but have readableEnded
so replaced with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was a typo.
|
||
// Initiate the server close before client data is sent, this will | ||
// test if the server properly waits for the stream to finish | ||
server.close(common.mustCall()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server.close(common.mustCall()); | |
server.close(); |
Feel free to keep the callback, it is not wrong, but in my opinion it is not useful for the purpose of the test, it is only additional overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense, done the changes
stream.on('close', common.mustCall(() => { | ||
assert.strictEqual(stream.writableFinished, true); | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
// Make an initial request | ||
const req = client.request({ ':path': '/' }); | ||
|
||
req.on('response', common.mustCall()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
req.on('response', common.mustCall()); |
I don't think this is useful for the purpose of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done the changes
assert.strictEqual(data, 'hello'); | ||
// Close the server as client is idle now | ||
setImmediate(() => { | ||
server.close(common.mustCall()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server.close(common.mustCall()); | |
server.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@pandeykushagra51 We should wait for @lpinca to re-review, but once that's sorted then there's nothing else required from your side, we'll do the steps to wrap it up and merge it. The CI tests can be a little flaky, but I don't think there's anything related to this change that's failing there, so nothing to worry about. We'll re-run those as required and sort that out, and then this PR will be landed. |
My comments/requests have been addressed. I do not approve because I think it does not belong to core. The argument for consistency with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with a nit
lib/internal/http2/core.js
Outdated
if (sessions.size > 0) { | ||
sessions.forEach((session) => { | ||
session.close(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use a for...of
loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
hey @lpinca @pimterry @mcollina I found an issue with graceful http2 session closure and implemented fix at #57808 . I will be very happy to optimise and improve it further in case of any scope and will be eagerly waiting for your feedback, Sorry for putting it here but I think this is a potential bug (may be security related) so thought of informing here to fast forward process. Thankyou |
This change adds proper tracking of HTTP/2 server sessions to ensure they are gracefully closed when the server is shut down. It implements:
Breaking Change: any client trying to create new requests on existing connections will not be able to do so once server close is initiated
Fixes: #57611
Refs: https://datatracker.ietf.org/doc/html/rfc7540#section-9.1
Refs: https://nodejs.org/api/http.html#serverclosecallback
More Details:
Purpose
This PR implements proper tracking and graceful shutdown of HTTP/2 sessions when a server is closed. It addresses the gap between the documented behavior of server.close() and its actual implementation for HTTP/2 servers. Also if server have fired close, we should allow it to close as early as possible to free up system resources quickly. This change ensure that once server wants to close, goaway frame will be sent to every open session(connection) so that client get to know that server started shutdown process and client should not send any new request on existing connection.
Current Issues
Implementation Details
Behavior Clarification
According to the Node.js documentation for
server.close()
:This PR ensures that HTTP/2 servers follow this behavior by:
Closing existing HTTP/2 sessions gracefully, which means:
This implementation aligns with the HTTP/2 protocol specification for connection management as described in RFC 7540 Section 9.1.
API Impact
This is technically a breaking change, as clients attempting to create new requests on existing HTTP/2 connections will be unable to do so once server.close() is called. However, this behavior now correctly matches the documented behavior for HTTP servers.
Testing
This PR includes tests to verify that HTTP/2 sessions are properly tracked and gracefully shut down when the server is closed.
Newly added test files:
Modified test files: