Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

http: Optimize queued client aborts#7533

Closed
kpdecker wants to merge 2 commits intonodejs:masterfrom
kpdecker:http-queued-abort
Closed

http: Optimize queued client aborts#7533
kpdecker wants to merge 2 commits intonodejs:masterfrom
kpdecker:http-queued-abort

Conversation

@kpdecker
Copy link
Copy Markdown

Avoid sending unsent data and destroying otherwise legitimate sockets
for requests that are aborted while still in the agent queue. This
reduces stress on upstream systems who will likely respond to the
request but client app already knows that it will be dropped on the
floor and also helps avoid killing keep-alive connections.

@Nodejs-Jenkins
Copy link
Copy Markdown

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • kpdecker

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@indutny
Copy link
Copy Markdown
Member

indutny commented May 2, 2014

Are you sure that there is no chance of the data coming out before .abort()?

@kpdecker
Copy link
Copy Markdown
Author

kpdecker commented May 3, 2014

This only changes the behavior of abort if there is not a socket. Since there is no socket, no data could have been sent unless I'm missing something.

@trevnorris trevnorris added the http label May 7, 2014
@trevnorris
Copy link
Copy Markdown

Two things.

  1. Please add ClientRequest.prototype.aborted = false; just below the function ClientRequest() declaration to set the default value.

  2. Please include a test for this change in functionality.

Ping me when this is done. :)

Avoid sending unsent data and destroying otherwise legitimate sockets
for requests that are aborted while still in the agent queue. This
reduces stress on upstream systems who will likely respond to the
request but client app already knows that it will be dropped on the
floor and also helps avoid killing keep-alive connections.
@kpdecker
Copy link
Copy Markdown
Author

kpdecker commented May 7, 2014

@trevnorris Done.

Rebuilding after switching between 0.10 and master apparently takes a bit of time :)

@trevnorris
Copy link
Copy Markdown

Thanks much.

@tjfontaine This LGTM, but mind giving it a glance to make sure there was nothing I missed?

@tjfontaine
Copy link
Copy Markdown

Can we make this by default undefined and then when set Date.now()? Seems like it would be helpful information when debugging (wallclock usage aside)

@kpdecker
Copy link
Copy Markdown
Author

The aborted flag?

@tjfontaine
Copy link
Copy Markdown

@kpdecker yup

Adds truthy flag that can be used for debugging as well. Per review by @tjfontaine.
@kpdecker
Copy link
Copy Markdown
Author

@tjfontaine Updated

@tjfontaine
Copy link
Copy Markdown

Thanks, squashed and landed in d0c7d93

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants