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

Create Local handle for new Buffers to avoid GC #20

Merged
merged 1 commit into from
Jul 21, 2014
Merged

Create Local handle for new Buffers to avoid GC #20

merged 1 commit into from
Jul 21, 2014

Conversation

rvagg
Copy link
Contributor

@rvagg rvagg commented Jul 18, 2014

fixes #19

The lack of a Local handle inside the HandleScope where the new Buffer objects are being created means that the GC can kick in while you're still working on them and clean up the handles that were originally created for them. This change introduces explicit handles each time you Buffer::New() which will automatically be assigned to the HandleScope in use.

@jedisct1
Copy link
Contributor

👍

@trevnorris
Copy link

Wow, forgot how ugly the Buffer API is in 0.10.

Also, Buffer::New(size) doesn't zero fill the memory. So it's possible to have random bytes wandering around in the allocated memory. Don't know the code that we'll, but are you writing to the entire Buffer or just part of it.

@trevnorris
Copy link

@rvagg I'm pretty sure name->handle_ is a Persistent which implies it lives outside the life cycle of the HandleScope. So I'm missing why the creation of a Local would fix the problem.

@rvagg
Copy link
Contributor Author

rvagg commented Jul 18, 2014

@warner
Copy link

warner commented Jul 18, 2014

I can confirm that this patch fixes the corruption problem I saw in #19, at least it made it through 10M cycles whereas before I was seeing corruption after only a few thousand. I know nothing about the memory-lifetime tools here, but memory usage appears stable, so I don't think it's keeping the memory around forever.

@rvagg
Copy link
Contributor Author

rvagg commented Jul 19, 2014

the addition shouldn't have any impact on memory: Local handles only increase the reference count of an object within the HandleScope for which it is created, outside of that it's free to be garbage collected if the count drops to zero. Buffer objects have an implicit Persistent reference from Node's ObjectWrap which floats free of HandleScopes but it's marked as weak when you first "wrap" an object so it's effectively up for grabs for garbage collection as soon as it has no references.

paixaop added a commit that referenced this pull request Jul 21, 2014
Create Local handle for new Buffers to avoid GC
@paixaop paixaop merged commit 7f00bf3 into paixaop:master Jul 21, 2014
@paixaop
Copy link
Owner

paixaop commented Jul 21, 2014

Thank you for the fix. this bug was hunting me for some time now.

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.

corrupt SecretBox ciphertext
5 participants