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

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-actions Bot 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 thread test/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-actions Bot 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-actions Bot mentioned this pull request May 25, 2023

CI Reliability 2023-05-25 nodejs/reliability#576

Open

21 tasks

github-actions Bot 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:main May 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

Uh oh!

Uh oh!

Conversation

mcollina commented May 21, 2023

Uh oh!

nodejs-github-bot commented May 21, 2023

Uh oh!

mcollina May 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