http2: fix graceful session close

[pandeykushagra51]

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)

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[pandeykushagra51]

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]

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]

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]

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]

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]

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]

Some tests/linting are failing.

I think the proposed optimization would be useful.

[pandeykushagra51]

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.

[pandeykushagra51]

Some tests/linting are failing.

I think the proposed optimization would be useful.

hey @mcollina I have implemented following strategy for optimisation:

1. 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.
2. 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]

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

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/57808
✔  Done loading data for nodejs/node/pull/57808
----------------------------------- PR info ------------------------------------
Title      http2: fix graceful session close (#57808)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     pandeykushagra51:http2/fix-graceful-session-close -> nodejs:main
Labels     c++, lib / src, author ready, needs-ci
Commits    7
 - http2: fix graceful session close
 - test: fixed request-response handling
 - test: fix lint errors
 - http2: Reduces overhead during HTTP/2 session closure by minimizing c…
 - http2: fixed cpp lint error
 - test: fixed incorrect http2 stream close test failure
 - resolved PR comment: fixed names and added description
Committers 1
 - Kushagra Pandey <pandeykushagra51@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/57808
Fixes: https://github.com/nodejs/node/issues/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>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/57808
Fixes: https://github.com/nodejs/node/issues/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>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 09 Apr 2025 19:55:01 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/57808#pullrequestreview-2763424555
   ✔  - Tim Perry (@pimterry): https://github.com/nodejs/node/pull/57808#pullrequestreview-2772755449
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/57808#pullrequestreview-2779922468
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-04-19T13:58:01Z: https://ci.nodejs.org/job/node-test-pull-request/66365/
- Querying data for job/node-test-pull-request/66365/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 57808
From https://github.com/nodejs/node
 * branch                  refs/pull/57808/merge -> FETCH_HEAD
✔  Fetched commits as 733e0fc2c4d0..d54f12c3e6ca
--------------------------------------------------------------------------------
Auto-merging src/env_properties.h
Auto-merging src/node_http2.cc
[main 3ee6e4a2df] http2: fix graceful session close
 Author: Kushagra Pandey <pandeykushagra51@gmail.com>
 Date: Thu Apr 10 00:52:44 2025 +0530
 5 files changed, 77 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-http2-session-graceful-close.js
[main b82d2b606a] test: fixed request-response handling
 Author: Kushagra Pandey <pandeykushagra51@gmail.com>
 Date: Thu Apr 10 02:04:51 2025 +0530
 1 file changed, 13 insertions(+), 12 deletions(-)
[main b1ce0f2f42] test: fix lint errors
 Author: Kushagra Pandey <pandeykushagra51@gmail.com>
 Date: Fri Apr 11 07:51:49 2025 +0530
 1 file changed, 9 insertions(+), 9 deletions(-)
Auto-merging src/node_http2.cc
[main d5fe338f23] http2: Reduces overhead during HTTP/2 session closure by minimizing callbacks 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
 Author: Kushagra Pandey <pandeykushagra51@gmail.com>
 Date: Fri Apr 11 10:22:13 2025 +0530
 3 files changed, 57 insertions(+), 5 deletions(-)
Auto-merging src/node_http2.cc
[main 7b4f4af3eb] http2: fixed cpp lint error
 Author: Kushagra Pandey <pandeykushagra51@gmail.com>
 Date: Fri Apr 11 20:02:25 2025 +0530
 2 files changed, 4 insertions(+), 5 deletions(-)
[main 54ebdf9810] test: fixed incorrect http2 stream close test failure
 Author: Kushagra Pandey <pandeykushagra51@gmail.com>
 Date: Sun Apr 13 01:23:30 2025 +0530
 1 file changed, 11 insertions(+), 6 deletions(-)
Auto-merging src/env_properties.h
Auto-merging src/node_http2.cc
[main 91dd7af93e] resolved PR comment: fixed names and added description
 Author: Kushagra Pandey <pandeykushagra51@gmail.com>
 Date: Tue Apr 15 19:27:02 2025 +0530
 4 files changed, 11 insertions(+), 10 deletions(-)
   ✔  Patches applied
There are 7 commits in the PR. Attempting autorebase.
Rebasing (2/14)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
http2: fix graceful session close
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>
[detached HEAD dfdf720beb] http2: fix graceful session close

Author: Kushagra Pandey <pandeykushagra51@gmail.com>

Date: Thu Apr 10 00:52:44 2025 +0530

5 files changed, 77 insertions(+), 2 deletions(-)

create mode 100644 test/parallel/test-http2-session-graceful-close.js

Rebasing (3/14)

Rebasing (4/14)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test: fixed request-response handling
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>
[detached HEAD be60606027] test: fixed request-response handling

Author: Kushagra Pandey <pandeykushagra51@gmail.com>

Date: Thu Apr 10 02:04:51 2025 +0530

1 file changed, 13 insertions(+), 12 deletions(-)

Rebasing (5/14)

Rebasing (6/14)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test: fix lint errors
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>
[detached HEAD f420dc58ca] test: fix lint errors

Author: Kushagra Pandey <pandeykushagra51@gmail.com>

Date: Fri Apr 11 07:51:49 2025 +0530

1 file changed, 9 insertions(+), 9 deletions(-)

Rebasing (7/14)

Rebasing (8/14)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

http2: Reduces overhead during HTTP/2 session closure by minimizing callbacks 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
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>
[detached HEAD dab7e54743] http2: Reduces overhead during HTTP/2 session closure by minimizing callbacks 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

Author: Kushagra Pandey <pandeykushagra51@gmail.com>

Date: Fri Apr 11 10:22:13 2025 +0530

3 files changed, 57 insertions(+), 5 deletions(-)

Rebasing (9/14)

Rebasing (10/14)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

http2: fixed cpp lint error
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>
[detached HEAD a4d32709a5] http2: fixed cpp lint error

Author: Kushagra Pandey <pandeykushagra51@gmail.com>

Date: Fri Apr 11 20:02:25 2025 +0530

2 files changed, 4 insertions(+), 5 deletions(-)

Rebasing (11/14)

Rebasing (12/14)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test: fixed incorrect http2 stream close test failure
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>
[detached HEAD a0f468c3d5] test: fixed incorrect http2 stream close test failure

Author: Kushagra Pandey <pandeykushagra51@gmail.com>

Date: Sun Apr 13 01:23:30 2025 +0530

1 file changed, 11 insertions(+), 6 deletions(-)

Rebasing (13/14)

Rebasing (14/14)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

resolved PR comment: fixed names and added description
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>
[detached HEAD 037a14d2a3] resolved PR comment: fixed names and added description

Author: Kushagra Pandey <pandeykushagra51@gmail.com>

Date: Tue Apr 15 19:27:02 2025 +0530

4 files changed, 11 insertions(+), 10 deletions(-)

Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/14550420536

[pandeykushagra51]

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]

Landed in 2acc8bc

[pandeykushagra51]

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]

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