http2: fix graceful session close by pandeykushagra51 · Pull Request #57808 · nodejs/node

pandeykushagra51 · 2025-04-09T19:55:01Z

Fix issue where session.close() prematurely destroys the session when response.end() was called with an empty payload while active http2 streams still existed. This change ensures that sessions are closed gracefully only after all http2 streams complete and clients properly receive the GOAWAY frame as per the HTTP/2 spec.

Refs: https://nodejs.org/api/http2.html\#http2sessionclosecallback
Fixes: #57809

Update:

Please refer detailed explanation below to know the intention behind updating test/parallel/test-http2-client-rststream-before-connect.js test.
#57808 (comment)

Fix issue where session.close() prematurely destroys the session when response.end() was called with an empty payload while active http2 streams still existed. This change ensures that sessions are closed gracefully only after all http2 streams complete and clients properly receive the GOAWAY frame as per the HTTP/2 spec. Refs: https://nodejs.org/api/http2.html\#http2sessionclosecallback

nodejs-github-bot · 2025-04-09T19:55:06Z

Review requested:

@nodejs/http2
@nodejs/net

pandeykushagra51 · 2025-04-09T20:10:41Z

Problem:

When a client calls response.end() with an empty payload, Node.js sends header only frames to nghttp2 as expected, and the JavaScript layer assumes these frames will be successfully transmitted post that. If session.close() is called immediately afterward, the Http2Session class doesn't recognize any active streams (as it only tracks data frames and push promises as active streams), and begins terminating the underlying socket. However, when the C++ layer subsequently attempts to transmit the queued header frames, it fails because the socket has already been closed, resulting in transmission errors and clients never receiving essential GOAWAY frames and response header.

Solution:

The solution ensures proper graceful session closure by delaying socket termination until all data in the nghttp2 queue/buffer is fully processed. This is implemented by:
Adding checks for nghttp2_session_want_write and nghttp2_session_want_read before closing the underlying socket
If nghttp2 has pending data to transmit, postponing the socket closure
Implementing a callback mechanism to notify the JavaScript layer when all writes have completed
Only proceeding with socket closure when all transmission conditions are satisfied
This approach guarantees that all frames, including header-only responses and GOAWAY frames, are properly delivered to the client before connection termination, ensuring robust HTTP/2 protocol compliance.

pandeykushagra51 · 2025-04-09T20:14:39Z

I will be very happy to update the current solution in case of any scope of improvement and would love to take input to optimise this further.

pandeykushagra51 · 2025-04-09T20:21:02Z

Optimization Proposal for the current fix

Current Issue:

The JavaScript layer is being called after each write operation to underlying socket.
This creates overhead with repetitive calls to onstreamaftrwrite_string()

Proposed Solution

Flag-Based Approach:
Set a flag in the C++ layer when JavaScript initiates a graceful closure
C++ layer checks this flag during write operations.

Responsibility Shift:

When the flag is set, make the C++ layer responsible for notifying the JavaScript layer
Notification happens either:
After nghttp2 completes all read and write operations (i.e hasPendingData return false)

Benefits:

Reduces repetitive calls to onstreamaftrwrite_string()
Improves performance by minimizing call between js and cpp layer.

Trade-off:

Implementation will be more complex
Solution will be longer but more efficient

pandeykushagra51 · 2025-04-09T20:30:07Z

-    session[kMaybeDestroy](err);
+    closeSession(session, NGHTTP2_NO_ERROR, err);


This change is done as improvement. This is because when underlying socket is closed there is no need for looking at graceful closure of session by calling [kMaybeDestroy] instead we can immediately call closeSession which will handle all cleanup operation.

pandeykushagra51 · 2025-04-09T21:00:10Z

One of the simplest approach is to delay socket destruction to next iteration of event loop i.e call inside setImmediate but I don't think this will address underlying problem

codecov · 2025-04-10T06:54:48Z

Codecov Report

Attention: Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (67786c1) to head (d54f12c).
Report is 59 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_http2.cc	91.89%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57808      +/-   ##
==========================================
+ Coverage   90.21%   90.23%   +0.02%     
==========================================
  Files         630      630              
  Lines      185524   185737     +213     
  Branches    36387    36414      +27     
==========================================
+ Hits       167378   167609     +231     
+ Misses      11037    11009      -28     
- Partials     7109     7119      +10

Files with missing lines	Coverage Δ
lib/internal/http2/core.js	`95.56% <100.00%> (+<0.01%)`	⬆️
src/node_http2.h	`91.66% <100.00%> (+0.25%)`	⬆️
src/node_http2.cc	`83.76% <91.89%> (+0.33%)`	⬆️

... and 52 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.

mcollina · 2025-04-10T09:29:03Z

Some tests/linting are failing.

I think the proposed optimization would be useful.

pandeykushagra51 · 2025-04-10T10:10:36Z

Some tests/linting are failing.

I think the proposed optimization would be useful.

Fine, then I will be implementing more optimised solution as stated above.
Also regarding failing test case, I am running the test on MacOS with below system

Darwin Mac.lan 24.3.0 Darwin Kernel Version 24.3.0: Thu Jan  2 20:24:24 PST 2025; root:xnu-11215.81.4~3/RELEASE_ARM64_T6030 arm64

And all tests are passed every time, so I am a bit confused why it is failing on CI.
I will check for rest of the OS and submit report here.

…allbacks between C++ and JavaScript layers. Added graceful_close_initiated_ flag which will ensure that JS layer will be notified only when session close is initiated and centralized notification logic in CheckAndNotifyJSAfterWrite() to only notify JavaScript when there's no pending data (nghttp2_session_want_write/read return 0). Previously, excessive callbac

pandeykushagra51 · 2025-04-11T05:00:16Z

Some tests/linting are failing.

I think the proposed optimization would be useful.

hey @mcollina I have implemented following strategy for optimisation:

Introduced a graceful session closed flag in cpp layer which will ensure that the callback will only be called if session close is initiated by js layer.
Added logic to call js layer only if nghttp2_wants_read and nghttp2_wants_write is false

Both of the above things when combined, will ensure that the new callback onstreamafterwrite_string will only be called once during the whole lifecycle of a session.

Regarding Failing tests, I am able to reproduce this occasionally i.e once in 10 run but not able to find root cause yet. I will be investigating further about RC.

mcollina

lgtm

nodejs-github-bot · 2025-04-11T07:40:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66195/

mcollina

lgtm

nodejs-github-bot · 2025-04-11T15:15:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66199/

pandeykushagra51 · 2025-04-19T16:19:33Z

Hey guys could you please check the last commit-queue.

Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

nodejs-github-bot · 2025-04-19T16:36:05Z

Landed in 2acc8bc

pandeykushagra51 · 2025-04-19T16:46:24Z

Thanks a lot @lpinca @jasnell @mcollina @pimterry.
It was really great working on node. Hope to have some more impactful contribution in near future.

pandeykushagra51 · 2025-04-19T16:49:25Z

Also please take a look at #57586.
it is also having commit-queue-failed status but have all checks passed and 3 approval.

Fix issue where session.close() prematurely destroys the session when response.end() was called with an empty payload while active http2 streams still existed. This change ensures that sessions are closed gracefully only after all http2 streams complete and clients properly receive the GOAWAY frame as per the HTTP/2 spec. Refs: https://nodejs.org/api/http2.html\#http2sessionclosecallback PR-URL: #57808 Fixes: #57809 Refs: https://nodejs.org/api/http2.html%5C#http2sessionclosecallback Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Apr 9, 2025

pandeykushagra51 mentioned this pull request Apr 9, 2025

graceful http2 session closure with empty body #57809

Closed

pandeykushagra51 mentioned this pull request Apr 9, 2025

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

Closed

pandeykushagra51 commented Apr 9, 2025

View reviewed changes

test: fixed request-response handling

0f36493

pandeykushagra51 added 2 commits April 11, 2025 07:51

test: fix lint errors

f26c10f

mcollina approved these changes Apr 11, 2025

View reviewed changes

mcollina requested review from jasnell and pimterry April 11, 2025 07:34

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 11, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 11, 2025

http2: fixed cpp lint error

87e9b8b

mcollina approved these changes Apr 11, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 11, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 11, 2025

lpinca added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Apr 19, 2025

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 19, 2025

nodejs-github-bot merged commit 2acc8bc into nodejs:main Apr 19, 2025

hildjj mentioned this pull request Apr 24, 2025

closeAllConnections on http2.Http2Server and http2.Http2SecureServer #55459

Closed

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

github-actions Bot mentioned this pull request May 19, 2025

2025-05-21, Version 22.16.0 'Jod' (LTS) #58388

Merged

marco-ippolito mentioned this pull request Jun 5, 2025

v20.19.3 proposal #58588

Merged

ghost mentioned this pull request Jun 8, 2025

### Notable Changes autofix-ci/action#28

Closed

robobun mentioned this pull request Oct 15, 2026

node:http2: return {} from session.state without a handle, keep connecting true until the socket is ready oven-sh/bun#44816

Open

		session[kMaybeDestroy](err);
		closeSession(session, NGHTTP2_NO_ERROR, err);

Conversation

pandeykushagra51 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update:

Uh oh!

nodejs-github-bot commented Apr 9, 2025

Uh oh!

pandeykushagra51 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem:

Solution:

Uh oh!

pandeykushagra51 commented Apr 9, 2025

Uh oh!

pandeykushagra51 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Optimization Proposal for the current fix

Current Issue:

Proposed Solution

Responsibility Shift:

Benefits:

Trade-off:

Uh oh!

pandeykushagra51 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

pandeykushagra51 commented Apr 9, 2025

Uh oh!

codecov Bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina commented Apr 10, 2025

Uh oh!

pandeykushagra51 commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pandeykushagra51 commented Apr 11, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 11, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 11, 2025

Uh oh!

pandeykushagra51 commented Apr 19, 2025

Uh oh!

nodejs-github-bot commented Apr 19, 2025

Uh oh!

pandeykushagra51 commented Apr 19, 2025

Uh oh!

pandeykushagra51 commented Apr 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pandeykushagra51 commented Apr 9, 2025 •

edited

Loading

pandeykushagra51 commented Apr 9, 2025 •

edited

Loading

pandeykushagra51 commented Apr 9, 2025 •

edited

Loading

codecov Bot commented Apr 10, 2025 •

edited

Loading

pandeykushagra51 commented Apr 10, 2025 •

edited

Loading