test: Verify request payload is uploaded consistently by awwright · Pull Request #34066 · nodejs/node

awwright · 2020-06-26T06:21:43Z

This adds several HTTP tests testing for the behavior identified in #27880:

test-http-27880-bug.js — shows how, if you make a DELETE request with a payload, the payload will actually be written to the server where a second HTTP/1.1 request is expected; if the payload is a valid HTTP message, it will be interpreted as a second request by the server. (But the first request is sent with Connection: close so shouldn't the server close the connection by this point?)
test-http-27880-chunked.js — tests that the default behavior for ClientRequest#write should be a chunked encoding.
test-http-27880-content-length.js — tests that the default behavior for ClientRequest#end should be a standard message, with an automatically computed Content-Length header.
test-http-27880-empty.js — tests that the default behavior for ClientRequest#end should have no payload headers if there's no response body

Naming for the test files will probably need to be reconsidered; suggestions welcome.

Refs: #27880

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included

commit message follows commit guidelines

aduh95 · 2020-10-14T13:08:14Z

@awwright there are some failing tests which keep this from landing. Do you still want to work on this?

github-actions · 2020-10-14T13:08:35Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

awwright · 2020-10-14T22:49:38Z

@aduh95 Those failed tests are bugs in the HTTP implementation that need to be fixed. However I'm not sure what the process is for doing this, exactly.

aduh95 · 2020-10-14T23:00:07Z

I think the process is to move those tests to test/known_issues.

awwright · 2020-10-15T01:12:38Z

@aduh95 I rebased and moved the tests, I'm hoping that is sufficient?

aduh95 · 2020-10-15T07:19:23Z

Regarding the naming of the file, what about:

test-http-request-method-delete-payload.js
test-http-clientrequest-write-chunked.js
test-http-clientrequest-end-contentlength.js
test-http-clientrequest-end-empty-response-body.js

awwright · 2020-10-15T08:08:09Z

That seems reasonable.

It's not obvious in how the files are written, but I'm trying to test the behavior in both directions, too. I actually forget off-hand if the bug is in requests only, or both request and responses.

aduh95 · 2020-11-08T12:17:05Z

@awwright Do you want to rename those files so we can move forward with this?

github-actions · 2020-11-08T12:17:23Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

github-actions · 2021-01-09T00:17:56Z

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

ronag · 2021-04-25T20:21:15Z

@aduh95 @dnlup @awwright any chance of revival?

ronag · 2021-04-25T20:21:35Z

@nodejs/http

dnlup · 2021-04-26T07:19:33Z

I am a little swamped atm. I don't think I am going to be able to work on it now. Sorry.

awwright · 2022-10-03T19:16:14Z

What else exactly does this need? Can we not merge a test that's showing an actual, live bug?

Node.js seems to change how it is uploaded based on the method, but HTTP doesn't make any distinction. Co-authored-by: Austin Wright <aaa@bzfx.net> Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot · 2024-05-11T23:01:07Z

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

nodejs-github-bot · 2024-05-12T07:15:01Z

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

nodejs-github-bot · 2024-05-12T17:22:45Z

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

aduh95 · 2024-05-12T19:15:33Z

Landed in d4e365f

Node.js seems to change how it is uploaded based on the method, but HTTP doesn't make any distinction. Co-authored-by: Austin Wright <aaa@bzfx.net> Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #34066 Refs: #27880 Reviewed-By: James M Snell <jasnell@gmail.com>

Node.js seems to change how it is uploaded based on the method, but HTTP doesn't make any distinction. Co-authored-by: Austin Wright <aaa@bzfx.net> Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#34066 Refs: nodejs#27880 Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 26, 2020

awwright mentioned this pull request Jun 26, 2020

No content length on DELETE and OPTIONS #27880

Open

awwright force-pushed the issue-27880 branch from b4ce665 to 08a2d5d Compare June 26, 2020 06:43

ronag added the http Issues or PRs related to the http subsystem. label Jun 26, 2020

awwright force-pushed the issue-27880 branch 2 times, most recently from 3a9b24a to 02d39b8 Compare June 26, 2020 08:02

jasnell approved these changes Jun 26, 2020

View reviewed changes

aduh95 added the stalled Issues and PRs that are stalled. label Oct 14, 2020

aduh95 removed the stalled Issues and PRs that are stalled. label Oct 14, 2020

awwright force-pushed the issue-27880 branch from 02d39b8 to c722a16 Compare October 15, 2020 01:08

awwright requested a review from jasnell October 15, 2020 01:11

aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020

github-actions bot closed this Jan 9, 2021

ronag reopened this Apr 25, 2021

ronag added the review wanted PRs that need reviews. label Apr 25, 2021

Trott removed the stalled Issues and PRs that are stalled. label Dec 15, 2021

98lenvi mentioned this pull request Sep 26, 2022

test: Verify request payload is uploaded consistently #44801

Closed

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

awwright mentioned this pull request Nov 30, 2023

HTTP requests with no Host header accepted #39033

Closed

aduh95 force-pushed the issue-27880 branch 2 times, most recently from 7cb53b7 to b753797 Compare May 11, 2024 17:20

aduh95 force-pushed the issue-27880 branch from b753797 to 1f659d4 Compare May 11, 2024 17:31

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024

aduh95 merged commit d4e365f into nodejs:main May 12, 2024

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

Uh oh!

Conversation

awwright commented Jun 26, 2020 • edited by aduh95 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

aduh95 commented Oct 14, 2020

Uh oh!

github-actions bot commented Oct 14, 2020

Uh oh!

awwright commented Oct 14, 2020

Uh oh!

aduh95 commented Oct 14, 2020

Uh oh!

awwright commented Oct 15, 2020

Uh oh!

aduh95 commented Oct 15, 2020

Uh oh!

awwright commented Oct 15, 2020

Uh oh!

aduh95 commented Nov 8, 2020

Uh oh!

github-actions bot commented Nov 8, 2020

Uh oh!

github-actions bot commented Jan 9, 2021

Uh oh!

ronag commented Apr 25, 2021

Uh oh!

ronag commented Apr 25, 2021

Uh oh!

dnlup commented Apr 26, 2021

Uh oh!

awwright commented Oct 3, 2022

Uh oh!

nodejs-github-bot commented May 11, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

aduh95 commented May 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

awwright commented Jun 26, 2020 •

edited by aduh95

Loading