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

http: Optimize queued client aborts #7533

Closed
wants to merge 2 commits into from

Conversation

kpdecker
Copy link

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

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
Member

indutny commented May 2, 2014

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

@kpdecker
Copy link
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

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

Thanks much.

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

@tjfontaine
Copy link

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
Author

The aborted flag?

@tjfontaine
Copy link

@kpdecker yup

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

@tjfontaine Updated

@tjfontaine
Copy link

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