Closed Xanewok closed 6 days ago
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
hardhat | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Jun 17, 2024 9:47am |
Latest commit: 6d235b14c725b94f6bef70b9b6fff44deb1fecd4
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
@fvictorio can I get a double check that it's also not used externally like _node._vm
or the _callOverrideCallback
?
Uhm, I'm not sure we can remove this. This might cause eventAdapter
to be garbage collected and the listeners to be dropped.
I'm not sure at all if that could really happen, because this things are always tricky in javascript, but I'd say we err on the side of paranoia and keep it. We should add a comment about this in the field though.
@Wodann do you remember if that's the reason you added this to EdrProviderWrapper
?
@Wodann do you remember if that's the reason you added this to
EdrProviderWrapper
?
There is a comment here explaining things. Maybe the garbage collector prevents destroying the _eventAdapter
because it's part of a callback, even if not stored on the EDR provider. I'm not 100% sure though.
In theory it should always be traced (in the GC sense) through the lambda and thus kept alive but I see now that the subscriber callback is moved to the Rust side, so it depends on the fact how it's implemented exactly.
From the cursory glance it seems that:
ProviderData
SubscriberCallback
with a create_threadsafe_function
napi-rs callArc<ThreadsafeFunctionHandle>
Drop
, which calls napi_release_threadsafe_function
which leads me to believe that it should be reachable by v8 and thus kept alive; I'd be really surprised if the func argument to napi_create_threadsafe_function
would have to be kept alive separately and not added to the 'alive' roots by napi/v8 itself.
Going a bit deeper into the node internals:
ThreadSafeFunction
classv8::Persistent
class, which the docs state is alive for the engine's lifetimeThe last part is a bit hand-wave-y as I don't want to get so deep into how exactly environment/resource management is implemented (there's a lot of ground and minutiae to cover!)
All in all, this looks safe to do. WDYT?
Yes, overall it seems like a safe bet. I'm not sure how this would manifest if our assumption is wrong, but I think it would mean that filters would stop working "after a while". Worth keeping in mind in case we see that behavior in the future, although it does seem highly unlikely.
Seems to be a leftover from #4747 and it looks to be unused now as we create a new instance of
EdrProviderEventAdapter
explicitly increate()
and move it into lambda.Feel free to disregard if that's not true and/or we want to keep it.