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

Remove leftover _eventAdapter from EdrProviderWrapper #5400

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jun 17, 2024

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

Seems to be a leftover from #4747 and it looks to be unused now as we create a new instance of EdrProviderEventAdapter explicitly in create() and move it into lambda.

Feel free to disregard if that's not true and/or we want to keep it.

Copy link

vercel bot commented Jun 17, 2024

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

Copy link

changeset-bot bot commented Jun 17, 2024

⚠️ No Changeset found

Latest commit: 6d235b1

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

Seems to be a leftover from #4747 and it looks to be unused now as we create a new instance of `EdrProviderEventAdapter` explicitly in `create()` and move it into lambda.

Feel free to disregard if that's not true and/or we want to keep it.
@Xanewok Xanewok added the no changeset needed This PR doesn't require a changeset label Jun 17, 2024
Copy link
Member

@kanej kanej left a comment

Choose a reason for hiding this comment

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

LGTM.

@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 19, 2024

@fvictorio can I get a double check that it's also not used externally like _node._vm or the _callOverrideCallback?

@Xanewok Xanewok requested a review from fvictorio June 19, 2024 09:53
@fvictorio
Copy link
Member

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.

@fvictorio
Copy link
Member

@Wodann do you remember if that's the reason you added this to EdrProviderWrapper?

@Wodann
Copy link
Member

Wodann commented Jun 20, 2024

@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.

@kanej kanej removed their assignment Jun 27, 2024
@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 28, 2024

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:

  1. it's moved into ProviderData
  2. as SubscriberCallback with a create_threadsafe_function napi-rs call
  3. which internally holds Arc<ThreadsafeFunctionHandle>
  4. only that implements 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:

  1. https://github.com/nodejs/node/blob/0062d5a07632c1333f35e509af002c3b2f81cf18/src/node_api.cc#L1315 creates a new ThreadSafeFunction class
  2. https://github.com/nodejs/node/blob/0062d5a07632c1333f35e509af002c3b2f81cf18/src/node_api.cc#L228 During construction, it resets the persistent handle to the v8 function via the v8::Persistent class, which the docs state is alive for the engine's lifetime

The 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?

@fvictorio
Copy link
Member

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.

@Xanewok Xanewok merged commit 01dd5b6 into main Jul 2, 2024
76 of 77 checks passed
@Xanewok Xanewok deleted the Xanewok-patch-1 branch July 2, 2024 11:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no changeset needed This PR doesn't require a changeset
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants