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.

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

http: check for handle before running asyncReset() #14419

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Jul 21, 2017

Checklist

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

http

If an uninitialized or user supplied Socket is in the freeSockets list of
the Agent it would automatically attempt to run ._handle.asyncReset(),
but would throw from those not existing. Guard against that by first
checking that they exist.

Fixes: #13539
Refs: #13352

CI: https://ci.nodejs.org/job/node-test-commit/11301/

refack: added ref to 13352

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 21, 2017
@mscdex mscdex added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 22, 2017
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

I don't know the http module well enough to comment on this. I trust @refack's opinion.

@refack
Copy link
Contributor

refack commented Jul 22, 2017

I don't know the http module well enough to comment on this. I trust @refack's opinion.

😳 Just so you stop trusting me, I wanted to add this back in #13839 (comment) but forgot to open a new PR.

const agent = new http.Agent({
keepAlive: true,
});
const socket = new net.Socket();
Copy link
Contributor

Choose a reason for hiding this comment

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

@trevnorris Re your comment #13045 (comment)
Could you add a test case for a Socket with a fake _handle?

@trevnorris
Copy link
Contributor Author

trevnorris commented Jul 24, 2017

If an uninitialized or user supplied Socket is in the freeSockets list
of the Agent it would automatically attempt to run
._handle.asyncReset(), but would throw from those not existing. Guard
against that by first checking that they exist.

PR-URL: nodejs#14419
Fixes: nodejs#13539
Refs: nodejs#13352
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack refack merged commit 93f47b1 into nodejs:master Jul 25, 2017
@trevnorris
Copy link
Contributor Author

@refack Thanks for landing this.

addaleax pushed a commit that referenced this pull request Jul 27, 2017
If an uninitialized or user supplied Socket is in the freeSockets list
of the Agent it would automatically attempt to run
._handle.asyncReset(), but would throw from those not existing. Guard
against that by first checking that they exist.

PR-URL: #14419
Fixes: #13539
Refs: #13352
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Aug 16, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'asyncReset' of null
8 participants