http: send implicit headers on HEAD with no body by mcollina · Pull Request #48108 · nodejs/node

mcollina · 2023-05-21T23:17:45Z

If we respond to a HEAD request with a body, we ignore all writes. However, we must still include all implicit headers.

Fixes a regressions introduced in
#47732.

If we respond to a HEAD request with a body, we ignore all writes. However, we must still include all implicit headers. Fixes a regressions introduced in nodejs#47732. Signed-off-by: Matteo Collina <hello@matteocollina.com>

nodejs-github-bot · 2023-05-21T23:17:49Z

Review requested:

@nodejs/http
@nodejs/net

mcollina · 2023-05-21T23:18:24Z

 const server = http.createServer(function(req, res) {
 res.writeHead(200);
- res.end();
+ res.end('FAIL'); // broken: sends FAIL from hot path.


This was a spurious change introduced by the previous PR.

nodejs-github-bot · 2023-05-21T23:20:41Z

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

…-headers.js Co-authored-by: cjihrig <cjihrig@gmail.com>

nodejs-github-bot · 2023-05-22T10:09:54Z

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

nodejs-github-bot · 2023-05-24T13:41:56Z

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

gerrard00 · 2023-05-25T18:31:01Z

Appreciate the fix, sorry for not catching that.

nodejs-github-bot · 2023-05-26T09:06:31Z

Commit Queue failed

- Loading data for nodejs/node/pull/48108
✔ Done loading data for nodejs/node/pull/48108
----------------------------------- PR info ------------------------------------
Title http: send implicit headers on HEAD with no body (#48108)
Author Matteo Collina (@mcollina)
Branch mcollina:fix-regression-from-47732 -> nodejs:main
Labels http, author ready, commit-queue-squash
Commits 2
- http: send implicit headers on HEAD with no body
- Update test/parallel/test-http-head-response-has-no-body-end-implicit…
Committers 2
- Matteo Collina - GitHub PR-URL: https://github.com/nodejs/node/pull/48108
Reviewed-By: Colin Ihrig Reviewed-By: Robert Nagy Reviewed-By: Paolo Insogna Reviewed-By: Marco Ippolito ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48108
Reviewed-By: Colin Ihrig Reviewed-By: Robert Nagy Reviewed-By: Paolo Insogna Reviewed-By: Marco Ippolito --------------------------------------------------------------------------------
⚠ Commits were pushed since the last review:
⚠ - Update test/parallel/test-http-head-response-has-no-body-end-implicit…
ℹ This PR was created on Sun, 21 May 2023 23:17:45 GMT
✔ Approvals: 4
✔ - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/48108#pullrequestreview-1435613010
✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/48108#pullrequestreview-1435740161
✔ - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/48108#pullrequestreview-1435797038
✔ - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/48108#pullrequestreview-1436063447
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2023-05-24T13:41:56Z: https://ci.nodejs.org/job/node-test-pull-request/51947/
- Querying data for job/node-test-pull-request/51947/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/5089120647

ronag

LGTM but I would prefer that if kRejectNonStandardBodyWrites is true we throw before sending headers as that can give a falls sense of success. While if kRejectNonStandardBodyWrites false we have the previous behaviour where we ignore the body but send the headers.

mcollina · 2023-05-26T12:04:24Z

I do not have time for further changes to this PR and we should fix the breaking change.

Do you prefer a revert of the original PR?

mcollina · 2023-05-26T12:04:45Z

(@ronag you did not approve this PR)

ronag · 2023-05-26T12:17:17Z

@MCOLLINE approved

nodejs-github-bot · 2023-05-26T12:38:26Z

Landed in 38b82b0

If we respond to a HEAD request with a body, we ignore all writes. However, we must still include all implicit headers. Fixes a regressions introduced in #47732. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #48108 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

If we respond to a HEAD request with a body, we ignore all writes. However, we must still include all implicit headers. Fixes a regressions introduced in nodejs#47732. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs#48108 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

http: send implicit headers on HEAD with no body
bb09c61
If we respond to a HEAD request with a body, we ignore all writes. However, we must still include all implicit headers. Fixes a regressions introduced in nodejs#47732. Signed-off-by: Matteo Collina <hello@matteocollina.com>

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels May 21, 2023

mcollina commented May 21, 2023
View reviewed changes

mcollina requested review from ShogunPanda and ronag May 21, 2023 23:18

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 21, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 21, 2023

cjihrig approved these changes May 21, 2023
View reviewed changes

Comment threadtest/parallel/test-http-head-response-has-no-body-end-implicit-headers.js Outdated

ronag approved these changes May 22, 2023
View reviewed changes

ShogunPanda approved these changes May 22, 2023
View reviewed changes

climba03003 mentioned this pull request May 22, 2023
skip test on broken node 20.2.0 fastify/fastify-reply-from#317
Merged
2 tasks

marco-ippolito approved these changes May 22, 2023
View reviewed changes

Update test/parallel/test-http-head-response-has-no-body-end-implicit…
ee6846c
…-headers.js Co-authored-by: cjihrig <cjihrig@gmail.com>

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2023

This was referenced May 23, 2023
CI Reliability 2023-05-23 nodejs/reliability#574
Open
CI Reliability 2023-05-24 nodejs/reliability#575
Open

github-actionsBot mentioned this pull request May 25, 2023
CI Reliability 2023-05-25 nodejs/reliability#576
Open
21 tasks

github-actionsBot mentioned this pull request May 26, 2023
CI Reliability 2023-05-26 nodejs/reliability#577
Open
21 tasks

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2023

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label May 26, 2023

ronag reviewed May 26, 2023
View reviewed changes

ronag approved these changes May 26, 2023
View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 26, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2023

nodejs-github-bot merged commit 38b82b0 into nodejs:mainMay 26, 2023

mcollina deleted the fix-regression-from-47732 branch May 26, 2023 12:38

This was referenced May 27, 2023
CI Reliability 2023-05-27 nodejs/reliability#578
Open
CI Reliability 2023-05-28 nodejs/reliability#579
Open
CI Reliability 2023-05-29 nodejs/reliability#580
Open
CI Reliability 2023-05-30 nodejs/reliability#581
Open

sapphi-red mentioned this pull request Jun 4, 2023
HEAD request to vite dev times out on Node v20.2.0 vitejs/vite#13383
Closed
7 tasks

targos mentioned this pull request Jun 4, 2023
v20.3.0 release proposal #48332
Merged

danielleadams mentioned this pull request Jul 10, 2023
v18.17.0 release proposal #48694
Merged

Navigation Menu

Uh oh!

Uh oh!

http: send implicit headers on HEAD with no body - #48108

http: send implicit headers on HEAD with no body#48108
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
mcollina:fix-regression-from-47732

mcollina commented May 21, 2023

Uh oh!

nodejs-github-bot commented May 21, 2023

Uh oh!

mcollinaMay 21, 2023

Uh oh!

nodejs-github-bot commented May 21, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented May 22, 2023

Uh oh!

nodejs-github-bot commented May 24, 2023

Uh oh!

gerrard00 commented May 25, 2023

Uh oh!

nodejs-github-bot commented May 26, 2023

Uh oh!

ronag left a comment

Uh oh!

mcollina commented May 26, 2023

Uh oh!

mcollina commented May 26, 2023

Uh oh!

ronag commented May 26, 2023

Uh oh!

nodejs-github-bot commented May 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Uh oh!

Conversation

mcollina commented May 21, 2023

Uh oh!

nodejs-github-bot commented May 21, 2023

Uh oh!

mcollinaMay 21, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 21, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented May 22, 2023

Uh oh!

nodejs-github-bot commented May 24, 2023

Uh oh!

gerrard00 commented May 25, 2023

Uh oh!

nodejs-github-bot commented May 26, 2023

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 26, 2023

Uh oh!

mcollina commented May 26, 2023

Uh oh!

ronag commented May 26, 2023

Uh oh!

nodejs-github-bot commented May 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants