Skip to content

http2: skip writeHead if stream is closed - #57686

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
islandryu:fix/http2WriteHead
Apr 4, 2025
Merged

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

Conversation

@islandryu

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.jsconsthttp2=require('http2');constserver=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.jsconsthttp2=require('http2');constclient=http2.connect('http://localhost:6000');constreq=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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-botnodejs-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

codecovBot commented Mar 30, 2025

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 linesCoverage Δ
lib/internal/http2/compat.js96.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.

@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2025

@mcollinamcollina 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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterrypimterry added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 3, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator

Landed in b140283

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
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 2, 2025
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
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
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
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 19, 2025
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
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-ippolitomarco-ippolito mentioned this pull request Jun 5, 2025
@ghostghost mentioned this pull request Jun 8, 2025
marco-ippolito pushed a commit that referenced this pull request Jun 10, 2025
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
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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.http2Issues or PRs related to the http2 subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

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

@islandryu@nodejs-github-bot@mcollina@lpinca@pimterry@RafaelGSS@jakecastelli