Fix memory leak on context shutdown#1856
Merged
ianhattendorf merged 5 commits intoOct 27, 2021
Merged
Conversation
Member
|
This should be rebased onto master for updated tests to run. |
AlexaXs
force-pushed
the
fix/memory-leak-when-closing-context
branch
from
de9b143 to
1e28f2d
Compare
There was a memory leak when v8 context is closed: the Nan::ObjectWrap objects created inside nodegit were not freed, since their WeakCallback were not triggered. To fix it, a new class has been added from which NodeGitWrapper objects inherit, so that they can be tracked in the nodegit::Context and freed when its cleanup handle is triggered. This new class allows for cheap tracking via Link/Unlink to a tracker list, allowing each NodeGitWrapper object to link/unlink itself while the normal GC is running, as well as for the nodegit::Context to keep a tracker list of them and free them in a 'owned objects first' way when closing. The ownership model and the fact that a NodeGitWrapper object might have multiple owners have also been considered.
This reverts commit b0e7440.
- add code to get the number of tracked objects. - export getNumberOfTrackedObjects for NodeGit addon.
so that they can be freed on context shutdown
AlexaXs
force-pushed
the
fix/memory-leak-when-closing-context
branch
from
1e28f2d to
e3c8dde
Compare
ianhattendorf
requested changes
Oct 26, 2021
ianhattendorf
left a comment
ianhattendorf
left a comment
Member
There was a problem hiding this comment.
This is looking great, just a few comments. Going to run some manual tests.
ianhattendorf
approved these changes
Oct 27, 2021
ianhattendorf
left a comment
ianhattendorf
left a comment
Member
There was a problem hiding this comment.
Local testing in Node and Electron checks out 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a memory leak when v8 context is closed: the NodeGitWrapper objects created (which inherit from Nan::ObjectWrap) during the execution of NodeGit were not freed when for example a worker thread was terminated.
To fix it, a new class has been added from which NodeGitWrapper objects inherit, so that they can be tracked in the nodegit::Context and freed when its cleanup handle is triggered.
This new class allows for cheap tracking via Link/Unlink to a tracker list, allowing each NodeGitWrapper object to link/unlink itself while the normal GC is running, as well as for the nodegit::Context to keep a tracker list of them and free them in a 'owned objects first' way when closing.