Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http: fix _dump regression #20088

Closed
wants to merge 1 commit into from
Closed

http: fix _dump regression #20088

wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Member

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
@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
Member Author

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

@apapirovski
Copy link
Member Author

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

@mcollina
Copy link
Member

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
Member Author

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
Member Author

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
Member Author

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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@apapirovski
Copy link
Member 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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@addaleax
Copy link
Member

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

@trivikr trivikr trivikr approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

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