Activity

1. probablycorey commented on Oct 13, 2014

   

   probablycorey

   on Oct 13, 2014

   Author

   More actions

   More info:

   node.js: v0.10.32
   os: OS X 10.9.4
2. johnhaley81 commented on Oct 13, 2014

   

   johnhaley81

   on Oct 13, 2014

   Collaborator

   More actions

   Thanks @probablycorey, I'm looking into this as I'm working in that area right now. Thanks for the test case!
3. johnhaley81 commented on Oct 13, 2014

   

   johnhaley81

   on Oct 13, 2014

   Collaborator

   More actions

   Yeah, I'm getting that to give me the same error on my end. I'm getting about 100 or so commits until it happens though. Were you seeing it like almost immediately or after a lot of commits have been processed?
4. probablycorey commented on Oct 13, 2014

   

   probablycorey

   on Oct 13, 2014

   Author

   More actions

   Thanks @johnhaley81! It consistently fails on the 94th commit for me. If I add console.log(commit.sha()) it fails at the 86th commit 1cfccb4.

   From what I can tell, the more I log about the commit the quicker it crashes.
5. johnhaley81 commented on Oct 13, 2014

   

   johnhaley81

   on Oct 13, 2014

   Collaborator

   More actions

   Ok, I narrowed it down to how we're handling things in the destructor in our generated C++ classes. This was super helpful! Thanks @probablycorey!
6. johnhaley81 commented on Oct 14, 2014

   

   johnhaley81

   on Oct 14, 2014

   Collaborator

   More actions

   So this affects more than just RevWalk.

   This is a behavior of how we pass back internal C objects on a V8 wrapper to JS. Right now we have a function call that creates a new V8 wrapper class that holds the C object that the user can then use as they wish. The problem is that each time that function is called we create a new wrapper. Once those variables go out of scope they are deleted. Unfortunately they are all pointing to the same value on the parent so the parent also loses that value when those wrapper classes are destroyed.

   What we need to do is for each V8 wrapper class that contains other C objects, we need to have private variables that wrap those C objects and when the caller wants to access them we get the value of those private wrappers classes and pass that back to the caller. This way there is only ever one wrapper class for the internal value of the parent and it's also scoped to the parent as well.

   The issue with the above example is the 2 console.log lines. Namely:

   console.log(commit.author().email());
   console.log(commit.author().name());
   

   What's happening here is that each call to commit.author() is creating a wrapper for the author and returning that value. When these go out of scope and garbage collection triggers it will delete the first instance and then try to delete the second. But when it gets to the second instance the value is already deleted so the garbage collector will throw an error. You can actually fix the above example by changing those lines to

   var author = commit.author();
   console.log(author.email());
   console.log(author.name());
   

   Which should run the example with no errors.

   I'm closing this issue and starting up a new one for the refactoring of how we store internal objects and pass them back to the caller.

   Thanks for the great test case @probablycorey! It helped a lot!
7. johnhaley81

   closed this as completedon Oct 14, 2014
8. maxkorp commented on Oct 24, 2014

   

   maxkorp

   on Oct 24, 2014

   Collaborator

   More actions

   @probablycorey This is fixed in master now.
9. probablycorey commented on Oct 26, 2014

   

   probablycorey

   on Oct 26, 2014

   Author

   More actions

   Awesome, @maxkorp 😄 ‼️