Skip to content

http: fix _dump regression#20088

Closed
apapirovski wants to merge 1 commit intonodejs:masterfrom
apapirovski:fix-http-consuming-regression
Closed

http: fix _dump regression#20088
apapirovski wants to merge 1 commit intonodejs:masterfrom
apapirovski:fix-http-consuming-regression

Conversation

@apapirovski
Copy link
Contributor

@apapirovski apapirovski commented Apr 16, 2018

A recent set of changes removed _consuming tracking from server incoming messages which ensures that IncomingMessage#_dump only runs if the user has never attempted to read the incoming data. Fix by reintroducing _consuming which tracks whether IncomingMessage#_read was ever successfully called.

Long-term it would be nice if it was possible to reply with status code 413 to a request with a long payload (without actually handling all of that payload) and not have the http client throw write EPIPE error while not receiving any of that response.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@apapirovski apapirovski added the http Issues or PRs related to the http subsystem. label Apr 16, 2018
@apapirovski apapirovski requested a review from mcollina April 16, 2018 22:49
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 16, 2018
@apapirovski apapirovski added this to the 10.0.0 milestone Apr 16, 2018
@apapirovski
Copy link
Contributor Author

apapirovski commented Apr 16, 2018

This should probably be in 10.x to avoid a regression in http.

@apapirovski
Copy link
Contributor Author

apapirovski commented Apr 16, 2018

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

@mcollina
Copy link
Member

mcollina commented Apr 16, 2018

What would this fix? Is there a bug to track that? What is the regression?

Side note, before this there was zero test coverage of _dump behavior. And a test to see _dump called as been added.

@apapirovski
Copy link
Contributor Author

apapirovski commented Apr 16, 2018

What would this fix? Is there a bug to track that? What is the regression?

Any situation that's not covered by the current resumeScheduled check (which is the majority), such as the test attached here. Basically, if the user starts doing some reading and pauses but then calls end, we shouldn't just dump the request data since they've shown interest in it. (And yes, it was untested.)

I'm still fixing one failure on the CI. Should have a fix shortly.

@apapirovski apapirovski force-pushed the fix-http-consuming-regression branch from 91cd8f8 to 83a0fee Compare April 16, 2018 23:28
@apapirovski
Copy link
Contributor Author

apapirovski commented Apr 16, 2018

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

Had to add back the manipulation of the _readableState.readingMore prop but was able to leave out the .read modification. Would like to find a way around using _readableState but for now think it's probably more important to fix the regression.

@apapirovski apapirovski force-pushed the fix-http-consuming-regression branch from 83a0fee to 8a9ef4f Compare April 16, 2018 23:36
@apapirovski
http: fix _dump regression
32d6472 
A recent set of changes removed _consuming tracking from server
incoming messages which ensures that _dump only runs if the
user has never attempted to read the incoming data. Fix by
reintroducing _consuming which tracks whether _read was ever
sucessfully called.
@apapirovski apapirovski force-pushed the fix-http-consuming-regression branch from 8a9ef4f to 32d6472 Compare April 16, 2018 23:37
mcollina
mcollina approved these changes Apr 17, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I think we should document this somewhere.

LGTM on code (we’ll need to remove those _readableState access later on).

@mcollina mcollina mentioned this pull request Apr 17, 2018
Closed
2 tasks
lpinca
lpinca approved these changes Apr 17, 2018
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

Good job.

@apapirovski
Copy link
Contributor Author

apapirovski commented Apr 19, 2018

Landed in 3d480dc

@apapirovski apapirovski deleted the fix-http-consuming-regression branch April 19, 2018 06:57
apapirovski added a commit that referenced this pull request Apr 19, 2018
@apapirovski
http: fix _dump regression
3d480dc 
A recent set of changes removed _consuming tracking from server
incoming messages which ensures that _dump only runs if the
user has never attempted to read the incoming data. Fix by
reintroducing _consuming which tracks whether _read was ever
successfully called.

PR-URL: #20088
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@apapirovski
Copy link
Contributor Author

apapirovski commented Apr 19, 2018 •
edited
Loading

@jasnell if you can pull this in into 10.0.0 that would be ideal. That way we can avoid a potential regression in http.

jasnell pushed a commit that referenced this pull request Apr 19, 2018
@apapirovski @jasnell
http: fix _dump regression
299da1f 
A recent set of changes removed _consuming tracking from server
incoming messages which ensures that _dump only runs if the
user has never attempted to read the incoming data. Fix by
reintroducing _consuming which tracks whether _read was ever
successfully called.

PR-URL: #20088
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@addaleax
Copy link
Member

addaleax commented Jun 29, 2018

Backported the test part for v8.x in #21595

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@trivikr trivikr trivikr approved these changes

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@apapirovski @mcollina @addaleax @jasnell @lpinca @trivikr @nodejs-github-bot