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: add session tracking and graceful server close #57668

Conversation

pandeykushagra51
Copy link

@pandeykushagra51 pandeykushagra51 commented Mar 28, 2025

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

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

  • Currently, HTTP/2 servers are at the mercy of clients to close connections.
  • There is no mechanism to notify clients that the server wants to shut down.
  • Misbehaving/Non-compliant clients can keep connections open indefinitely, preventing proper server shutdown.
  • Clients can continue to initiate new requests on existing connections even when the server is trying to close.
  • Start of server shutdown can starve indefinitely, leading to processes that cannot terminate cleanly and causing resource leaks.

Implementation Details

  • Added a kSessions symbol and SafeSet to track active HTTP/2 sessions.
  • Implemented session registration when sessions are created.
  • Created a closeAllSessions helper function to initiate graceful shutdown of all tracked sessions
  • Updated Http2Server and Http2SecureServer close methods to use this functionality

Behavior Clarification

According to the Node.js documentation for server.close():

server.close stops the server from accepting new connections and closes all connections connected to this server which are not sending a request or waiting for a response.

This PR ensures that HTTP/2 servers follow this behavior by:
Closing existing HTTP/2 sessions gracefully, which means:

  • When session.close() is called, a GOAWAY frame is sent to notify clients that the server wants to close the session
  • No new streams (requests) can be initiated on existing connections
  • Existing streams can continue writing/reading data until completion
  • In-flight requests will be allowed to complete
  • Sessions will be terminated after all active streams complete

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.

Modified test files:

  • test-http2-compat-serverresponse-statusmessage-property-set.js: After server close behavior is updated, this test started to fail. The failure happened because the request from client is under pendingAck state so the session is not yet initiated, as server close is fired during this time and hence client didn't received response which leads to test failure. Now the test is updated to fire server close once the request is complete. As this unit test, test that client should receive http2 status message response, so it is good to close the server only after the request/response cycle is completed. This line tells that pending stream will be cancelled once session close is emitted.
  • test-http2-compat-serverresponse-statusmessage-property.js: Same as above.
  • test-http2-capture-rejection.js: Server close should be fired after server have sent push promise frame.

This replaces #57586

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
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Mar 28, 2025
Copy link

codecov bot commented Mar 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (af75d04) to head (12de1eb).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #57668   +/-   ##
=======================================
  Coverage   90.23%   90.23%           
=======================================
  Files         630      630           
  Lines      185055   185084   +29     
  Branches    36221    36226    +5     
=======================================
+ Hits       166984   167015   +31     
+ Misses      11043    11040    -3     
- Partials     7028     7029    +1     
Files with missing lines Coverage Δ
lib/internal/http2/core.js 95.67% <100.00%> (+0.06%) ⬆️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants