Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: be more aggressive to reply 400, 408 and 431 #44818

Merged

Conversation

ywave620
Copy link
Contributor

@ywave620 ywave620 commented Sep 29, 2022

Fix #37685, which is introduced in e8d7fed. It is too conservate (from the perspective from avoiding corrupting the client) to reply an error message.

Cases where we should reply an error message are covered in those 4 tests this PR modified, while the case where the socket should be immediately destroyed is covered in test/parallel/test-http-header-badrequest.js.

As long as data of the in-flight response is not yet written
to the socket, we can reply an error response without corrupting
the client.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Sep 29, 2022
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

LGTM once CI failures are fixed

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

lib/_http_server.js Outdated Show resolved Hide resolved
fix comment

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2022
@nodejs-github-bot
Copy link
Collaborator

@ywave620
Copy link
Contributor Author

ywave620 commented Oct 4, 2022

Shall we proceed to get this PR landed?

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Oct 6, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 6, 2022
@nodejs-github-bot nodejs-github-bot merged commit 6475a87 into nodejs:main Oct 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 6475a87

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
As long as data of the in-flight response is not yet written
to the socket, we can reply an error response without corrupting
the client.

PR-URL: #44818
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error handling for HPE_HEADER_OVERFLOW in HTTP server does not take into account socket re-use
7 participants