-
Notifications
You must be signed in to change notification settings - Fork 67
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
src: keep reference buffer alive longer #55
Conversation
Keep a buffer written by WritePointer alive until the finalizers for the buffer to which the pointer has been run have been executed: Refs: node-ffi-napi#54 Signed-off-by: Michael Dawson <mdawson@devrus.com>
@addaleax any chance you can take a look at this? |
@addaleax I know you've been busy, but wondering if you might be able to take a look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhdawson Thanks for the ping. This creates memory leaks by adding hidden connection between Buffer
objects that remain present indefinitely, even if another “pointer” is written to the same location later.
This library generally leaves object lifetime management up to the user – yes, that’s hard to get right, but it’s also easy to get it wrong if we do it here in a generic fashion without being clear about what guarantees we make and what guarantees we don’t make.
As such, #54 seems to be ”working as expected” to me, even if I can see that it’s not what a user might expect.
If we do add a solution for this, it should:
- Not introduce memory leaks
- Be clearly documented, and in particular it should be clearly stated how it differs from the pre-N-API
ref
package
@addaleax thanks for taking a look. I saw the following code: exports.writePointer = function writePointer (buf, offset, ptr) {
debug('writing pointer to buffer', buf, offset, ptr);
exports._writePointer(buf, offset, ptr);
exports._attach(buf, ptr);
}; Which I thought was doing a similar association at the JS level. It looks to me like _attach adds a reference to ptr in buf, such that ptr won't be collected until after buf is collected. The comment in _attach says:
So based on that my understanding is that only once buf is collected by the gc, will the ptr buffer be able to be collected. This PR was trying to do the same thing but ensure the order in which finalizers would be run. When buf is collected, the finalizer will run which will delete the references and then allow the ptr to be collected. I understand the point about what happens if you write another pointer at the same offset but I can't find any code which removes the reference set in _attach so I was thinking that would already be the behavior but maybe I'm just missing something as I'm not familiar with the code. |
I can see that maybe the problem is that its changed for _writePointer as well as writePointer. If that is the issue then we can look at refactoring so the addition only applies to writePointer versus _writePointer as well. Does that make sense? |
Update to avoid changing behaviour for _writePointer and limit change to writePointer where objects were already being associated with the buffer. Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson In that case, wouldn’t it make sense to remove the JS |
@addaleax I pushed another commit to limit the change so that it only affects From what I can see The change in this PR should now just mean that the object is not eligible to be collected until after the finalizers for the buffer have been run which is tied to when the buffer is freed just like before. ie, no change to what is kept alive. To confirm this I wrote this simple test: var ref = require('ref-napi');
var st = require('ref-struct-di')(ref);
var at = require('ref-array-di')(ref);
var idx = 0
const mainBuffer = Buffer.alloc(10)
setInterval(() => {
const ptr = Buffer.alloc(100000);
ref.writePointer(mainBuffer, 0, ptr);
if (++idx % 1000 === 0) console.log(`${idx}th iteration`)
},1) Running that test without the changes from this PR results in the heap space as reported by --trace-gc continually increasing. If I remove the existing line This confirms that the existing implementation of writePointer already keeps all buffers that are written to the buffer alive until the buffer they are written is eligible for collection even if they have been overwritten. I also validated that with the changes from this PR, using the example above with I think this confirms that this PR only affects the timing of when buffers written with I had also already run the original test in #54 PR under valgrind and --trace-gc with the PR and for the test case in #54 no memory is leaked. |
I terms of removing the '_attach' code I'm happy to do that, was just not sure at the time if it was doing anything else. @addaleax should I push another commit to remove the _attach call? |
remove _attach call in writePointer as it is no longer needed since a stronger version is done in the native code when true is passed as the fourth parameter
Pushed a commit to remove _attach as we can always drop that commit if that's not the right thing to do. |
I don't think the coverage failure should block this as its a result of removing code that was covered, versus adding code that was not covered. |
@mhdawson Thank you! This is published in |
@addaleax and thank you too :) |
I am still observing the issue with the fix also |
Keep a buffer written by WritePointer alive
until the finalizers for the buffer to which
the pointer has been run have been executed:
Refs: #54
Signed-off-by: Michael Dawson mdawson@devrus.com