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,async_hooks: keep resource object alive from socket #30196

Closed
wants to merge 1 commit into from

Conversation

@addaleax
Copy link
Member

addaleax commented Oct 31, 2019

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

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
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

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

This comment has been minimized.

Copy link
@Flarna

Flarna Oct 31, 2019

Member

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

This comment has been minimized.

Copy link
@addaleax

addaleax Oct 31, 2019

Author Member

@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
Flarna approved these changes Oct 31, 2019
@Flarna

This comment has been minimized.

Copy link
Member

Flarna commented Oct 31, 2019

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

This comment has been minimized.

Copy link
Member Author

addaleax commented Oct 31, 2019

@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

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 4, 2019

Copy link
Member

vdeturckheim left a comment

LGTM

@addaleax addaleax removed the review wanted label Nov 6, 2019
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 6, 2019

Landed in d26a74d

@addaleax addaleax closed this Nov 6, 2019
addaleax added a commit that referenced this pull request Nov 6, 2019
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 addaleax deleted the addaleax:async-hooks-domain-gc-30122 branch Nov 6, 2019
@HazAT

This comment has been minimized.

Copy link

HazAT commented Nov 7, 2019

@addaleax Hello and thank you from Sentry 👋 😄 🎉

MylesBorins added a commit that referenced this pull request Nov 17, 2019
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 BridgeAR mentioned this pull request Nov 19, 2019
targos added a commit that referenced this pull request Dec 1, 2019
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 BethGriggs mentioned this pull request Dec 9, 2019
MylesBorins added a commit that referenced this pull request Dec 17, 2019
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 BethGriggs mentioned this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.