Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Fix top-level v8::Locker #3038

Closed
piscisaureus opened this issue Mar 30, 2012 · 5 comments
Closed

Fix top-level v8::Locker #3038

piscisaureus opened this issue Mar 30, 2012 · 5 comments

Comments

@piscisaureus
Copy link

@laverdet

We reverted your commit in 4071815 because it made some repl tests crash in debug mode.

@laverdet
Copy link

Hey, so this had nothing to do with Locker. Locker makes v8 a little louder about errors while in debug mode, but by deleting the Locker all you're doing is hiding issues that exist already. In the vm module there exists many cases where you enter a context but don't exit it. I submitted a pull request that adds the Locker back yet again, and fixes the particular issue you mentioned.

#3039

But really you should avoid reverting this in the future. My first commit adding Locker did have an issue, but I'm confident that the implementation is now robust. It will make certain errors louder, but those errors would have led to more quiet memory leaks without the extra error checking.

My pull request adds a small wrapper class for locally-scoped persistent handles. You could use a similar pattern to simplify complicated Dispose() logic throughout node.

@laverdet
Copy link

Also the included PersistentScope class could be a little bit cleaner, replacing v8::Persistent entirely in cases where it makes sense. I'd be interested in working on cleaning up patterns like this throughout node if there's mutual interest amongst Joyent.

@laverdet
Copy link

/cc @bnoordhuis ;)

@laverdet
Copy link

+@isaacs: As per #3090 and #3042, could we please get 4071815 reverted?

@isaacs
Copy link

isaacs commented May 11, 2012

After a bit of back and forth this seems to be closed. @laverdet Can you please comment if there's still anything to do here, and I'll reopen in that case? Thanks.

@isaacs isaacs closed this as completed May 11, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants