GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: send implicit headers on HEAD with no body by mcollina · Pull Request #48108 · nodejs/node

Conversation

@mcollina

@mcollina mcollina commented May 21, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
fengmk2 and bricss reacted with rocket emoji
All reactions
  • 🚀 2 reactions
@mcollina
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 <[email protected]>
@nodejs-github-bot

nodejs-github-bot commented May 21, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels May 21, 2023
mcollina
mcollina commented May 21, 2023
View reviewed changes
Comment thread test/parallel/test-http-head-response-has-no-body-end.js
const server = http.createServer(function(req, res) {
res.writeHead(200);
res.end();
res.end('FAIL'); // broken: sends FAIL from hot path.

@mcollina mcollina May 21, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This was a spurious change introduced by the previous PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina
mcollina requested review from ShogunPanda and ronag May 21, 2023 23:18
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 21, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 21, 2023
@nodejs-github-bot

nodejs-github-bot commented May 21, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
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 Resolved

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag approved these changes May 22, 2023
View reviewed changes
ShogunPanda
ShogunPanda approved these changes May 22, 2023
View reviewed changes
@climba03003 climba03003 mentioned this pull request May 22, 2023
skip test on broken node 20.2.0 fastify/fastify-reply-from#317
Merged
2 tasks done
marco-ippolito
marco-ippolito approved these changes May 22, 2023
View reviewed changes
@mcollina @cjihrig
Update test/parallel/test-http-head-response-has-no-body-end-implicit…
ee6846c 
…-headers.js

Co-authored-by: cjihrig <[email protected]>
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 22, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 22, 2023
@nodejs-github-bot

nodejs-github-bot commented May 22, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced May 23, 2023
CI Reliability 2023-05-23 nodejs/reliability#574
Open
CI Reliability 2023-05-24 nodejs/reliability#575
Open
@nodejs-github-bot

nodejs-github-bot commented May 24, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request May 25, 2023
CI Reliability 2023-05-25 nodejs/reliability#576
Open
21 tasks
@gerrard00

gerrard00 commented May 25, 2023

Copy link
Copy Markdown
Contributor

Appreciate the fix, sorry for not catching that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request May 26, 2023
CI Reliability 2023-05-26 nodejs/reliability#577
Open
21 tasks
@mcollina mcollina added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed needs-ci PRs that need a full CI run. labels May 26, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 26, 2023
@nodejs-github-bot nodejs-github-bot added the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label May 26, 2023
@nodejs-github-bot

nodejs-github-bot commented May 26, 2023

Copy link
Copy Markdown
Collaborator
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag reviewed May 26, 2023
View reviewed changes

@ronag ronag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

mcollina commented May 26, 2023

Copy link
Copy Markdown
Member Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented May 26, 2023

Copy link
Copy Markdown
Member Author

(@ronag you did not approve this PR)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag approved these changes May 26, 2023
View reviewed changes
@ronag

ronag commented May 26, 2023

Copy link
Copy Markdown
Member

@MCOLLINE approved
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels May 26, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 26, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 38b82b0 into nodejs:main May 26, 2023
@nodejs-github-bot

nodejs-github-bot commented May 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 38b82b0
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina
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
targos pushed a commit that referenced this pull request May 30, 2023
@mcollina @targos
http: send implicit headers on HEAD with no body
60ce2bc 
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 <[email protected]>
PR-URL: #48108
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@sapphi-red 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 done
@targos targos mentioned this pull request Jun 4, 2023
v20.3.0 release proposal #48332
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@mcollina @danielleadams
http: send implicit headers on HEAD with no body
7e3e4cb 
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 <[email protected]>
PR-URL: #48108
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@mcollina @MoLow
http: send implicit headers on HEAD with no body
709e368 
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 <[email protected]>
PR-URL: nodejs#48108
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
v18.17.0 release proposal #48694
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@mcollina @Ceres6
http: send implicit headers on HEAD with no body
8152442 
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 <[email protected]>
PR-URL: nodejs#48108
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@mcollina @Ceres6
http: send implicit headers on HEAD with no body
405a39d 
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 <[email protected]>
PR-URL: nodejs#48108
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes
@cjihrig cjihrig cjihrig approved these changes
@ronag ronag ronag approved these changes
@marco-ippolito marco-ippolito marco-ippolito approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. http Issues and PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@mcollina @nodejs-github-bot @gerrard00 @ronag @ShogunPanda @cjihrig @marco-ippolito
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.