http: allow Content-Length header for 304 responses by BlackYoup · Pull Request #34835 · nodejs/node

BlackYoup · 2020-08-18T20:00:38Z

According to the HTTP spec, a 304 Not Modified response can have a Content-Length header:

A server MAY send a Content-Length header field in a 304 (Not Modified) response to a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a 200 (OK) response to the same request.

On the other hand, the spec doesn't allow a 304 response to have a body: A 304 response cannot contain a message-body; it is always terminated by the first empty line after the header fields.

Currently, the http request module will emit an aborted event if the response has a status code of 304 with a Content-Length header. This pull request fixes this behavior and the request can successfully finish.

This fixes issue #31037

I tried to integrate the fix in the best place I could find, let me know if there are better ways to fix this!

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-08-18T20:00:42Z

Review requested:

@nodejs/http
@nodejs/net

Trott · 2020-08-21T13:21:42Z

This could use some more reviews (and any thoughts on semver-ness). @nodejs/net @nodejs/http

nodejs-github-bot · 2020-08-23T11:16:38Z

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

github-actions · 2020-09-06T13:37:13Z

Commit Queue failed

- Loading data for nodejs/node/pull/34835 ✔ Done loading data for nodejs/node/pull/34835 ----------------------------------- PR info ------------------------------------ Title http: allow Content-Length header for 304 responses (#34835) Author Arnaud Lefebvre (@BlackYoup) Branch BlackYoup:issue-31037 -> nodejs:master Labels author ready, http, review wanted Commits 2 - http: allow Content-Length header for 304 responses - Update test/parallel/test-http-allow-content-length-304.js Committers 2 - Arnaud Lefebvre - GitHub PR-URL: https://github.com/nodejs/node/pull/34835 Reviewed-By: James M Snell Reviewed-By: Ricky Zhou <0x19951125@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/34835 Reviewed-By: James M Snell Reviewed-By: Ricky Zhou <0x19951125@gmail.com> -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-08-23T11:16:38Z: https://ci.nodejs.org/job/node-test-pull-request/32902/ - Querying data for job/node-test-pull-request/32902/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Tue, 18 Aug 2020 20:00:38 GMT ✔ Approvals: 2 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/34835#pullrequestreview-472590480 ✔ - Ricky Zhou (@rickyes): https://github.com/nodejs/node/pull/34835#pullrequestreview-473555162 -------------------------------------------------------------------------------- ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 34835 ✔ Downloaded patch to /home/runner/work/node/node/.ncu/34835/patch -------------------------------------------------------------------------------- Applying: http: allow Content-Length header for 304 responses Applying: Update test/parallel/test-http-allow-content-length-304.js ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
http: allow Content-Length header for 304 responses

Fixes #31037

PR-URL: #34835
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ricky Zhou 0x19951125@gmail.com

[detached HEAD 0a6e3bda] http: allow Content-Length header for 304 responses
Author: Arnaud Lefebvre a.lefebvre@outlook.fr
Date: Tue Aug 18 21:28:31 2020 +0200
2 files changed, 37 insertions(+)
create mode 100644 test/parallel/test-http-allow-content-length-304.js
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update test/parallel/test-http-allow-content-length-304.js

Co-authored-by: mscdex mscdex@users.noreply.github.com

PR-URL: #34835
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ricky Zhou 0x19951125@gmail.com

[detached HEAD e55739a9] Update test/parallel/test-http-allow-content-length-304.js
Author: Arnaud Lefebvre a.lefebvre@outlook.fr
Date: Wed Aug 19 01:07:11 2020 +0200
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/master.
✔ 0a6e3bdabb2ebd800237c34c68f022bf6a2f5b4c
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 3:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length
✖ e55739a9e0623284ee652b385aeab2ab7aa6e183
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 3:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length

ℹ Please fix the commit message and try again.

BlackYoup · 2020-09-08T20:39:07Z

Regarding the failure, should I squash both commits into one?

rickyes · 2020-09-10T14:54:16Z

Regarding the failure, should I squash both commits into one?

Of course it's better if you can. But it doesn't matter, we can use the tool to squash both commits into one and land it.

Fixes: nodejs#31037 PR-URL: nodejs#34835 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Trott · 2020-09-11T23:44:53Z

Landed in 23d6c42

Fixes: #31037 PR-URL: #34835 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Fixes: nodejs#31037 PR-URL: nodejs#34835 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

BlackYoup requested a review from a team as a code owner August 18, 2020 20:00

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 18, 2020

mscdex reviewed Aug 18, 2020
View reviewed changes

Comment threadtest/parallel/test-http-allow-content-length-304.js Outdated

mscdex reviewed Aug 18, 2020
View reviewed changes

Comment threadtest/parallel/test-http-allow-content-length-304.js Outdated

BlackYoup force-pushed the issue-31037 branch from f26f44b to 39c31a0Compare August 18, 2020 23:06

Trott added the review wanted PRs that need reviews. label Aug 21, 2020

jasnell approved these changes Aug 21, 2020
View reviewed changes

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

rickyes approved these changes Aug 24, 2020
View reviewed changes

rickyes added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 24, 2020

rickyes added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2020

github-actionsBot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 6, 2020

rickyes removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 10, 2020

trivikr approved these changes Sep 10, 2020
View reviewed changes

trivikr requested a review from mscdex September 10, 2020 22:06

This comment has been minimized.
Sign in to view

http: allow Content-Length header for 304 responses
23d6c42
Fixes: nodejs#31037 PR-URL: nodejs#34835 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Trott force-pushed the issue-31037 branch from 17b345e to 23d6c42Compare September 11, 2020 23:44

Trott merged commit 23d6c42 into nodejs:masterSep 11, 2020

ruyadorno mentioned this pull request Sep 21, 2020
V14.12.0 proposal #35285
Merged
4 tasks

Navigation Menu

Uh oh!

Uh oh!

http: allow Content-Length header for 304 responses - #34835

http: allow Content-Length header for 304 responses#34835
Trott merged 1 commit into
nodejs:masterfrom
BlackYoup:issue-31037

BlackYoup commented Aug 18, 2020

Uh oh!

nodejs-github-bot commented Aug 18, 2020

Uh oh!

Uh oh!

Uh oh!

Trott commented Aug 21, 2020

Uh oh!

nodejs-github-bot commented Aug 23, 2020•
edited by rickyes
Loading

Uh oh!

github-actionsBot commented Sep 6, 2020

PR-URL: #34835
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ricky Zhou 0x19951125@gmail.com

PR-URL: #34835
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ricky Zhou 0x19951125@gmail.com

Uh oh!

BlackYoup commented Sep 8, 2020

Uh oh!

rickyes commented Sep 10, 2020

Uh oh!

This comment has been minimized.

Trott commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Uh oh!

Conversation

BlackYoup commented Aug 18, 2020

Checklist

Uh oh!

nodejs-github-bot commented Aug 18, 2020

Uh oh!

Uh oh!

Uh oh!

Trott commented Aug 21, 2020

Uh oh!

nodejs-github-bot commented Aug 23, 2020• edited by rickyes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Sep 6, 2020

PR-URL: #34835 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ricky Zhou 0x19951125@gmail.com

PR-URL: #34835 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ricky Zhou 0x19951125@gmail.com

Uh oh!

BlackYoup commented Sep 8, 2020

Uh oh!

rickyes commented Sep 10, 2020

Uh oh!

This comment has been minimized.

Trott commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nodejs-github-bot commented Aug 23, 2020•
edited by rickyes
Loading

PR-URL: #34835
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ricky Zhou 0x19951125@gmail.com

PR-URL: #34835
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Ricky Zhou 0x19951125@gmail.com