Skip to content

http2: do no throw in writeHead if state.closed - #27682

Closed
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:http2-compat-state-closed-crash
Closed

http2: do no throw in writeHead if state.closed#27682
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:http2-compat-state-closed-crash

Conversation

@mcollina

@mcollinamcollina commented May 13, 2019

Copy link
Copy Markdown
Member

The http1 implementation does not throw if the connection is down.
The http2 compat implementation should do the same.

See: fastify/fastify-http-proxy#51.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented May 13, 2019

Copy link
Copy Markdown
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3508

@nodejs-github-botnodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label May 13, 2019
cjihrig
cjihrig approved these changes May 13, 2019
View reviewed changes
Comment threadtest/parallel/test-http2-compat-serverresponse-writehead.js Outdated
BridgeAR
BridgeAR reviewed May 13, 2019
View reviewed changes

@BridgeARBridgeAR 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.

The code change is LGTM. Should we not change the behavior in http to throw an error as well instead? Writing while already closed seems wrong?

@mcollina

mcollina commented May 14, 2019

Copy link
Copy Markdown
MemberAuthor

Should we not change the behavior in http to throw an error as well instead? Writing while already closed seems wrong?

That would break a significant number of applications unfortunately. Most applications and frameworks relies on the fact that calling writeHead() is safe.

@mcollinamcollina mentioned this pull request May 17, 2019
Closed
@mcollina
mcollina requested review from jasnell and removed request for jasnellMay 17, 2019 13:32
@mcollina
http2: do no throw in writeHead if state.closed
04c715f
The http1 implementation does not throw if the connection is down.
The http2 compat implementation should do the same.
See: fastify/fastify-http-proxy#51.
See: fastify/fastify#1494.
@mcollina
mcollinaforce-pushed the http2-compat-state-closed-crash branch from 003cef5 to 04c715fCompareMay 17, 2019 13:43
@mcollina

mcollina commented May 17, 2019

Copy link
Copy Markdown
MemberAuthor

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

1 similar comment
@nodejs-github-bot

nodejs-github-bot commented May 17, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 17, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 17, 2019

Copy link
Copy Markdown
Collaborator

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

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2019
@mcollina

mcollina commented May 18, 2019

Copy link
Copy Markdown
MemberAuthor

Landed in a49ab0f

@mcollina
mcollina deleted the http2-compat-state-closed-crash branch May 18, 2019 16:31
mcollina added a commit that referenced this pull request May 18, 2019
@mcollina
http2: do no throw in writeHead if state.closed
a49ab0f
The http1 implementation does not throw if the connection is down.
The http2 compat implementation should do the same.
See: fastify/fastify-http-proxy#51.
See: fastify/fastify#1494.
PR-URL: #27682
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 20, 2019
@mcollina@targos
http2: do no throw in writeHead if state.closed
a66b391
The http1 implementation does not throw if the connection is down.
The http2 compat implementation should do the same.
See: fastify/fastify-http-proxy#51.
See: fastify/fastify#1494.
PR-URL: #27682
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeARBridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
@mokimo

mokimo commented Nov 13, 2019•
edited
Loading

Copy link
Copy Markdown

I think it's still not completely fixed, getting the issue a bit later here:
https://github.com/nodejs/node/blob/master/lib/internal/http2/compat.js#L637

It does not throw when using http1, but does when using http2 compat implemenation

@mokimomokimo mentioned this pull request Nov 13, 2019
Closed
@mcollina

mcollina commented Nov 15, 2019

Copy link
Copy Markdown
MemberAuthor

@mokimo would you mind opening a new issue with instructions on how to reproduce? (ideally a server + client that calls it).

@mokimo

mokimo commented Nov 15, 2019•
edited
Loading

Copy link
Copy Markdown

If I can get a simple example running that'd be perfect... Been unsuccessful so far but will give you a quick ping you once I got something hands-on

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeARBridgeARBridgeAR left review comments
@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@cjihrigcjihrigcjihrig approved these changes
@apapirovskiapapirovskiAwaiting requested review from apapirovski
@targostargosAwaiting requested review from targos

Assignees

No one assigned

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mcollina@nodejs-github-bot@mokimo@jasnell@Trott@cjihrig@BridgeAR