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

Global drop queue for Root #700

Merged
merged 3 commits into from
Mar 22, 2021
Merged

Global drop queue for Root #700

merged 3 commits into from
Mar 22, 2021

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Mar 16, 2021

Why

Currently, dropping a Root will cause a panic instead of leaking the data. This can be problematic for users since there are many ways to accidentally leak a Root. Consider the following example:

  • A channel accepts closures to execute
  • A Root is created and captured by a closure
  • The closure is sent across the channel
  • The channel is closed

In this case, even though the user knows the Root won't be used, there's no easy way to get it back out to safely drop it.

What

This feature is flagged with napi-6. The existing panic behavior continues for older versions.

InstanceData

A private InstanceData type is added that uses napi_get_instance_data and napi_set_instance_data. This provides a place to stash module specific context for Neon.

Global drop queue

A global drop queue is added as a ThreadsafeFunction.

  • ThreadsafeFunction is used instead of EventQueue since we only need a static callback. Closures would require unnecessary overhead.
  • ThreadsafeFunction is maintained in an Arc to ensure it can outlive an unloaded module
  • The queue is unreferenced to prevent it from keeping the event loop running. Hanging references will be freed when the VM drops.

Additional

Added impl Object for JsBox that was missing.

Testing

It was really fun finding a creative way to test this behavior!

  • Create a struct Wrapper for holding a callback and an event queue
  • The wrapper is put in a JsBox which is then rooted and immediately leaked (dropped)
  • Instead of an idiomatic Finalize implementation, Wrapper has a Drop implementation that calls the callback. If this is called, we know that the Root reference count was dropped, allowing our struct to be dropped.
  • We force run the garbage collector after giving the drop queue an opportunity to execute

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

I think this is a great improvement. The code is just subtle enough that my main comments are about comments and docs.

@kjvalencik kjvalencik requested a review from dherman March 19, 2021 18:18
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants