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, async_hooks: Consider async_hooks implications of the API that HTTP Agent uses #13352

Closed
refack opened this issue May 31, 2017 · 3 comments
Closed

http, async_hooks: Consider async_hooks implications of the API that HTTP Agent uses #13352

refack opened this issue May 31, 2017 · 3 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem. wip Issues and PRs that are still a work in progress.

Comments

@refack
Copy link
Contributor

refack commented May 31, 2017

  • Version: master
  • Platform: *
  • Subsystem: http, async_hooks

The HTTP Agent is designed to be inherited and it's methods overridden. Currently there is non encapsulated async_hooks logic that can break if not re-implemented in overridden methods. We should better encapsulate this logic, and document for custom Agent implementors.

Ref: #13045
Ref: #13325

/cc @nodejs/async_hooks
@refack refack mentioned this issue May 31, 2017
Closed
3 tasks
@refack refack added async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem. wip Issues and PRs that are still a work in progress. labels May 31, 2017
@AndreasMadsen
Copy link
Member

Ref: #13539

@refack refack mentioned this issue Jul 3, 2017
Merged
3 tasks
refack added a commit to refack/node that referenced this issue Jul 3, 2017
@refack
http: guard against failed sockets creation
4b276e9 
PR-URL: nodejs#13839
Fixes: nodejs#13045
Fixes: nodejs#13831
Refs: nodejs#13352
Refs: nodejs#13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this issue Jul 3, 2017
@addaleax
http: guard against failed sockets creation
45a9c58 
PR-URL: nodejs#13839
Fixes: nodejs#13045
Fixes: nodejs#13831
Refs: nodejs#13352
Refs: nodejs#13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
@refack @addaleax
http: guard against failed sockets creation
2ce0de1 
PR-URL: #13839
Fixes: #13045
Fixes: #13831
Refs: #13352
Refs: #13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
@AndreasMadsen AndreasMadsen mentioned this issue Jul 17, 2017
Closed
3 tasks
addaleax pushed a commit that referenced this issue Jul 18, 2017
@refack @addaleax
http: guard against failed sockets creation
c652845 
PR-URL: #13839
Fixes: #13045
Fixes: #13831
Refs: #13352
Refs: #13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
@refack @Fishrock123
http: guard against failed sockets creation
959bdfd 
PR-URL: #13839
Fixes: #13045
Fixes: #13831
Refs: #13352
Refs: #13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
@trevnorris trevnorris mentioned this issue Jul 22, 2017
Merged
3 tasks
refack pushed a commit to trevnorris/node that referenced this issue Jul 25, 2017
@trevnorris @refack
http: check for handle before running asyncReset()
93f47b1 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this issue Jul 27, 2017
@trevnorris @addaleax
http: check for handle before running asyncReset()
751e873 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@Trott
Copy link
Member

Trott commented Apr 29, 2018

@refack Should this remain open? (I believe the answer is "yes" but I'm checking because it's been inactive for a long time....)

@apapirovski
Copy link
Member

Linked issues are closed. PRs landed. As far as I can tell, this is mostly addressed. If something isn't, please feel free to reopen but do provide more info on what exactly is missing or needs to be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@apapirovski @refack @AndreasMadsen @Trott