Skip to content

http2: emit close for aborted HEAD compat responses - #66310

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:http2-compat-head-close
Sep 28, 2026
Merged

nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:http2-compat-head-close

Conversation

@ronag

@ronag ronag commented Sep 26, 2026

Copy link
Copy Markdown
Member

The compat response defers 'finish' and 'close' for a HEAD request until response.end(), because the stream of a headers-only response closes as soon as the headers are sent. The same deferral also applied to a HEAD stream that closed before any response was sent, for example when the client cancelled it or the session was destroyed. Nothing was left to call end(), so the response never emitted 'close' and the abort could not be observed on it.

Defer only once the headers were sent, and otherwise close the response as for any other method. The writable side of a HEAD stream is finished from the start, so 'finish' is emitted only after the headers were sent, and an aborted HEAD response does not report success.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Sep 26, 2026
@ronag
ronag requested a review from mcollina September 26, 2026 10:27
@ronag
ronag force-pushed the http2-compat-head-close branch from 6f1a425 to 3e43327 Compare September 26, 2026 10:53
The compat response defers 'finish' and 'close' for a HEAD request
until response.end(), because the stream of a headers-only response
closes as soon as the headers are sent. The same deferral also applied
to a HEAD stream that closed before any response was sent, for example
when the client cancelled it or the session was destroyed. Nothing was
left to call end(), so the response never emitted 'close' and the abort
could not be observed on it.

Defer only once the headers were sent, and otherwise close the response
as for any other method. The writable side of a HEAD stream is finished
from the start, so 'finish' is emitted only after the headers were
sent, and an aborted HEAD response does not report success.

Assisted-by: Opus 5.5
Signed-off-by: Robert Nagy <ronagy@icloud.com>
@ronag
ronag force-pushed the http2-compat-head-close branch from 3e43327 to 8aed10d Compare September 26, 2026 10:53
@codecov

codecov Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.38%. Comparing base (45fef37) to head (8aed10d).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #66310   +/-   ##
=======================================
  Coverage   90.37%   90.38%           
=======================================
  Files         790      790           
  Lines      274497   274505    +8     
  Branches    52558    52555    -3     
=======================================
+ Hits       248065   248099   +34     
+ Misses      16922    16883   -39     
- Partials     9510     9523   +13     
Files with missing lines Coverage Δ
lib/internal/http2/compat.js 97.23% <100.00%> (+0.02%) ⬆️

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

@ronag
ronag requested a review from jasnell September 26, 2026 13:05
@ronag ronag 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 Sep 26, 2026

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 26, 2026
@panva panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 26, 2026
@github-actions 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 Sep 26, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 27, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 191a3b2 into nodejs:main Sep 28, 2026
93 of 94 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 191a3b2

@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 28, 2026
@aduh95 aduh95 added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. labels Sep 28, 2026
@aduh95

aduh95 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

IIUC this is based on #63249 and therefore should not be backported

HoonDongKang pushed a commit to HoonDongKang/node that referenced this pull request Sep 29, 2026
The compat response defers 'finish' and 'close' for a HEAD request
until response.end(), because the stream of a headers-only response
closes as soon as the headers are sent. The same deferral also applied
to a HEAD stream that closed before any response was sent, for example
when the client cancelled it or the session was destroyed. Nothing was
left to call end(), so the response never emitted 'close' and the abort
could not be observed on it.

Defer only once the headers were sent, and otherwise close the response
as for any other method. The writable side of a HEAD stream is finished
from the start, so 'finish' is emitted only after the headers were
sent, and an aborted HEAD response does not report success.

Assisted-by: Opus 5.5
Signed-off-by: Robert Nagy <ronagy@icloud.com>
PR-URL: nodejs#66310
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. http2 Issues and 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.

6 participants