FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http,async_hooks: keep resource object alive from socket by addaleax · Pull Request #30196 · nodejs/node · GitHub

/ node Public

http,async_hooks: keep resource object alive from socket - #30196

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:async-hooks-domain-gc-30122
ConversationCommits (1)ChecksFiles changed
Closed

http,async_hooks: keep resource object alive from socket#30196
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:async-hooks-domain-gc-30122

Conversation

addaleax commented Oct 31, 2019

Copy link
Copy Markdown
Member

If asyncReset() is used to specify an alternative resource object
to mark a re-used socket in the HTTP Agent implementation,
store that object and keep it alive, because domains rely on GC tracking
for resource objects to manage their own lifetimes, and previously that
resource object might have been garbage-collected too early, leading to
crashes.

Fixes: #30122

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

FYI @Flarna

http,async_hooks: keep resource object alive from socket
ec6d1e7 
If `asyncReset()` is used to specify an alternative resource object
to mark a re-used socket in the HTTP Agent implementation,
store that object and keep it alive, because domains rely on GC tracking
for resource objects to manage their own lifetimes, and previously that
resource object might have been garbage-collected too early, leading to
crashes.

Fixes: nodejs#30122
nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 31, 2019
addaleax mentioned this pull request Oct 31, 2019
Closed

nodejs-github-bot commented Oct 31, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Oct 31, 2019

Copy link
Copy Markdown
Collaborator

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

Flarna reviewed Oct 31, 2019
View reviewed changes
Comment thread lib/_http_agent.js
// AsyncWrap::AsyncReset() when the resource is not the AsyncWrap object
// itself. However, HTTPClientAsyncResource and HTTPServerAsyncResource
// hold on to other objects, inhibiting GC.
handle[kReusedHandle] = this;

Flarna Oct 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it ok that on second reuse of the handle the binding to the first ReusedHandle is removed?

addaleax Oct 31, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Flarna That only happens together with a destroy emitted before and a new ID assigned, so I’m pretty confident it’s safe to discard the first ReusedHandle at that point

Flarna commented Oct 31, 2019

Copy link
Copy Markdown
Member

Thanks for fixing this!
An alternative would be to revert 3d9d1ad as it seems the initial idea from diag summit to move towards resource based API is no longer in focus. I did that change because it was a low hanging fruit to move towards the direction.

But it seems that the use of AsyncHooks by domains is in general risky as there might be other users not using an AsyncWrap. For example if someone uses napi_async_init() with async_resource == nullptr a new object is created and used in init call but then no longer referenced.

addaleax commented Oct 31, 2019

Copy link
Copy Markdown
Member Author

@Flarna I do still feel that moving towards a resource-based API makes sense, it just needs somebody with sufficient time and resources to push it through.

Thanks for pointing out the N-API issue, that seems indeed like a problem of the kind already mentioned in the TODO comment here…

addaleax added the review wanted PRs that need reviews. label Nov 4, 2019

addaleax commented Nov 4, 2019

Copy link
Copy Markdown
Member Author

@nodejs/domains @nodejs/async_hooks

vdeturckheim approved these changes Nov 4, 2019
View reviewed changes

vdeturckheim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

addaleax added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. async_hooks Issues and PRs related to the async hooks subsystem. domain Issues and PRs related to the domain subsystem. labels Nov 5, 2019
addaleax removed the review wanted PRs that need reviews. label Nov 6, 2019

addaleax commented Nov 6, 2019

Copy link
Copy Markdown
Member Author

Landed in d26a74d

addaleax closed this Nov 6, 2019
addaleax added a commit that referenced this pull request Nov 6, 2019
http,async_hooks: keep resource object alive from socket
d26a74d 
If `asyncReset()` is used to specify an alternative resource object
to mark a re-used socket in the HTTP Agent implementation,
store that object and keep it alive, because domains rely on GC tracking
for resource objects to manage their own lifetimes, and previously that
resource object might have been garbage-collected too early, leading to
crashes.

Fixes: #30122

PR-URL: #30196
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax deleted the async-hooks-domain-gc-30122 branch November 6, 2019 11:34

HazAT commented Nov 7, 2019

Copy link
Copy Markdown

@addaleax Hello and thank you from Sentry 👋 😄 🎉

MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
http,async_hooks: keep resource object alive from socket
e59cc8a 
If `asyncReset()` is used to specify an alternative resource object
to mark a re-used socket in the HTTP Agent implementation,
store that object and keep it alive, because domains rely on GC tracking
for resource objects to manage their own lifetimes, and previously that
resource object might have been garbage-collected too early, leading to
crashes.

Fixes: #30122

PR-URL: #30196
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BridgeAR mentioned this pull request Nov 19, 2019
Merged
targos pushed a commit that referenced this pull request Dec 1, 2019
http,async_hooks: keep resource object alive from socket
2bd32a8 
If `asyncReset()` is used to specify an alternative resource object
to mark a re-used socket in the HTTP Agent implementation,
store that object and keep it alive, because domains rely on GC tracking
for resource objects to manage their own lifetimes, and previously that
resource object might have been garbage-collected too early, leading to
crashes.

Fixes: #30122

PR-URL: #30196
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs mentioned this pull request Dec 9, 2019
Closed
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
http,async_hooks: keep resource object alive from socket
6525142 
If `asyncReset()` is used to specify an alternative resource object
to mark a re-used socket in the HTTP Agent implementation,
store that object and keep it alive, because domains rely on GC tracking
for resource objects to manage their own lifetimes, and previously that
resource object might have been garbage-collected too early, leading to
crashes.

Fixes: #30122

PR-URL: #30196
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs mentioned this pull request Dec 23, 2019
Merged
Flarna mentioned this pull request Jan 30, 2020
Closed
4 tasks
sfount mentioned this pull request Feb 18, 2020
Merged
Flarna mentioned this pull request Mar 2, 2020
Closed
2 tasks
addaleax pushed a commit that referenced this pull request Mar 11, 2020
http, async_hooks: remove unneeded reference to wrapping resource
943887f 
Remove the reference from handle to the unique/wrapping resource
ReusedHandle as there is meanwhile a strong reference for all async
resources in place via AsyncWarp::resource_.

PR-URL: #32054
Refs: #30959
Refs: #30196
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
http, async_hooks: remove unneeded reference to wrapping resource
3119bbe 
Remove the reference from handle to the unique/wrapping resource
ReusedHandle as there is meanwhile a strong reference for all async
resources in place via AsyncWarp::resource_.

PR-URL: #32054
Refs: #30959
Refs: #30196
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
http, async_hooks: remove unneeded reference to wrapping resource
49a07f7 
Remove the reference from handle to the unique/wrapping resource
ReusedHandle as there is meanwhile a strong reference for all async
resources in place via AsyncWarp::resource_.

PR-URL: #32054
Refs: #30959
Refs: #30196
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
http, async_hooks: remove unneeded reference to wrapping resource
8e4b390 
Remove the reference from handle to the unique/wrapping resource
ReusedHandle as there is meanwhile a strong reference for all async
resources in place via AsyncWarp::resource_.

PR-URL: nodejs#32054
Refs: nodejs#30959
Refs: nodejs#30196
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 28, 2020
http, async_hooks: remove unneeded reference to wrapping resource
12b8345 
Remove the reference from handle to the unique/wrapping resource
ReusedHandle as there is meanwhile a strong reference for all async
resources in place via AsyncWarp::resource_.

PR-URL: #32054
Refs: #30959
Refs: #30196
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gireeshpunathil gireeshpunathil approved these changes
Flarna Flarna approved these changes
+1 more reviewer
vdeturckheim vdeturckheim approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. domain Issues and PRs related to the domain subsystem. http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read property 'enter' of undefined

6 participants

Back | FazBrowse Home | New Git URL