FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: skip writeHead if stream is closed by islandryu · Pull Request #57686 · nodejs/node · GitHub

/ node Public

http2: skip writeHead if stream is closed - #57686

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
islandryu:fix/http2WriteHead
Apr 4, 2025
ConversationCommits (1)ChecksFiles changed
Merged

http2: skip writeHead if stream is closed#57686
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
islandryu:fix/http2WriteHead

Conversation

islandryu commented Mar 30, 2025

Copy link
Copy Markdown
Member

Fixes: #57416

This issue can be reproduced by destroying the session immediately after the client sends a request.
You may occasionally see an error after running it a few times.

seq 100 | xargs -n1 -P100 node client.js
// server.js
const http2 = require('http2');
const server = http2.createServer((req, res) => {
  setTimeout(() => {
    res.writeHead(200, { 'content-type': 'text/plain' });
    res.end('Hello from HTTP/2 server!\n');
  }, 100); 
});

server.listen(6000, () => {});
// client.js
const http2 = require('http2');

const client = http2.connect('http://localhost:6000');
const req = client.request({ ':path': '/' });
setTimeout(() => {
    client.destroy();
}, 100);

If writeHead() is called after the server receives a GOAWAY frame but before it sends a RST_STREAM, the server may enter an error state.

After the GOAWAY frame is received, the http2Stream is immediately closed.
However, the response object (res) is only marked as closed after the RST_STREAM frame is sent.

Currently, writeHead() checks whether the response object is closed, but it does not check whether the stream itself has been closed.

how to fix

Even if the RST_STREAM frame hasn't been sent yet, it seems like it might already be queued when the http2Stream is closed.
So this change skips writeHead() when the stream is already closed.

nodejs-github-bot commented Mar 30, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

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

codecov Bot commented Mar 30, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (78af51c) to head (ddca2b5).
Report is 36 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57686      +/-   ##
==========================================
+ Coverage   90.22%   90.24%   +0.01%     
==========================================
  Files         630      630              
  Lines      185074   185074              
  Branches    36222    36221       -1     
==========================================
+ Hits       166985   167013      +28     
+ Misses      11041    11034       -7     
+ Partials     7048     7027      -21
Files with missing lines Coverage Δ
lib/internal/http2/compat.js 96.94% <100.00%> (ø)

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

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2025
mcollina approved these changes Mar 31, 2025
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

nodejs-github-bot commented Mar 31, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

nodejs-github-bot commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

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

islandryu force-pushed the fix/http2WriteHead branch from ec39cfd to ddca2b5 Compare April 2, 2025 13:41
pimterry added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Apr 3, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

nodejs-github-bot commented Apr 3, 2025

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 4, 2025

Copy link
Copy Markdown
Collaborator

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

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2025
nodejs-github-bot merged commit b140283 into nodejs:main Apr 4, 2025

nodejs-github-bot commented Apr 4, 2025

Copy link
Copy Markdown
Collaborator

Landed in b140283

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
http2: skip writeHead if stream is closed
9725a83 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
http2: skip writeHead if stream is closed
8f3aeea 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
http2: skip writeHead if stream is closed
7943581 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
http2: skip writeHead if stream is closed
7acbb85 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
http2: skip writeHead if stream is closed
2ce2764 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
github-actions Bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
http2: skip writeHead if stream is closed
b818924 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 5, 2025
http2: skip writeHead if stream is closed
39cf617 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
marco-ippolito mentioned this pull request Jun 5, 2025
Merged
ghost mentioned this pull request Jun 8, 2025
Closed
marco-ippolito pushed a commit that referenced this pull request Jun 10, 2025
http2: skip writeHead if stream is closed
93bed28 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 11, 2025
http2: skip writeHead if stream is closed
cdf3fa2 
Fixes: #57416
PR-URL: #57686
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
lpinca lpinca approved these changes
pimterry pimterry approved these changes
RafaelGSS RafaelGSS approved these changes
jakecastelli jakecastelli approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ERR_HTTP2_INVALID_STREAM thrown when attempting to end response when stream was destroyed

7 participants

Back | FazBrowse Home | New Git URL