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

Memory leak in Root #750

Merged
merged 2 commits into from
Jun 2, 2021
Merged

Memory leak in Root #750

merged 2 commits into from
Jun 2, 2021

Conversation

kjvalencik
Copy link
Member

Fixes a memory leak in Root reported in #746.

This PR includes two commits:

  • Delete N-API references when the count hits 0.
    This is the bug referenced above. It stems from a misunderstanding of N-API. It's not sufficient to decrement the reference count to 0, the reference must also be deleted.
  • Use an Option to store the reference instead of using ManuallyDrop.
    While not as extreme, the global event queue reference counter was always incrementing. It would never be freed, event after the context ended.

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.

This looks great, it's just a little bit subtle and I don't 100% understand yet. How did the ManuallyDrop logic prevent the queue reference count from decrementing, and how does the Option fix that?

src/handle/root.rs Show resolved Hide resolved
src/handle/root.rs Outdated Show resolved Hide resolved
@kjvalencik
Copy link
Member Author

kjvalencik commented Jun 2, 2021

This looks great, it's just a little bit subtle and I don't 100% understand yet. How did the ManuallyDrop logic prevent the queue reference count from decrementing, and how does the Option fix that?

@dherman Since Drop is recursive, ManuallyDrop prevents the fields from executing their Drop implementation as well. To drop the Arc would require some unsafe.

This changes the strategy so ManuallyDrop is no longer used. Instead, a None signals to the Drop implementation that nothing needs to be done. Since these fields are only used internally, the value will always be Some when a user interacts with it. The methods that take the value and replace it with None consume the struct. Since we control it, we can guarantee that only Drop will execute after the take.

@kjvalencik kjvalencik requested a review from dherman June 2, 2021 13:53
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.

Looks great!

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