Nested walks scatter memory and cause SEGFAULTS #72
Description
Activity
Can you please send us a coredump or run your code in gdb like and copy and paste for us?
% gdb --args node myscript.js > run (should segfault) > backtrace
Yep, thanks for taking a look!
https://gist.github.com/chbrown/5854153
This time the error is actually for a slightly different routine, but I think it's due to the same thing -- trying to walk too many diffs all at once.
I get the same error even without the --noopt* v8 options.
I use akka as a test repository because it's huge (11,000+ commits).
I'm on Mac OS X 10.8.4, up to date with v8 and node, etc., on homebrew.
OK, one thing I'm pretty certain about:
By "scatter memory", I mean that if I don't immediate call commit.sha(), but instead put the raw commit object in a queue, sometimes commit.message() and commit.sha() results won't line up correctly (they'll be contrary to what git log at the command line tells me).
You aren't allowed to do this with the current implementation of RevWalk since it uses a object pool.
NOTE: You can copy the message and the sha as a workaround, since they are just strings.
The more I think about this, and this is still sortof a guess -- I think RevWalk's current implementation is not suited to your task, as RevWalk uses an object pool and whatever it yields is not supposed to live longer than one invocation of the next() callback (so you shouldn't pass what it yields to another async function).
There isn't an easy workaround since commit.sha() and commit.oid() are both async.
I'm planning to rewrite these anyway. If you can wait a couple weeks for the new version to drop..
@faceleg yes, I think I'll wait for improvements. I'm not familiar with how memory management in C/C++ translates over to the javascript side, but I can see that it's hard to track down.
@nkallen what is now a custom evented walk (whether it's the repo's history() or difflist's deltas) seems like it'd be an apt candidate for {objectMode: true} ReadableStreams, though, again, I'm not sure how those are implemented on the C/C++ side.
If you're willing, please try the wip branch, which should fix this issue. Unfortunately there are many API changes for you to deal with though.
Use wip
Works much better, no SEGFAULTs so far, thanks! I'm curious why the commits aren't in order (say, the order that git log puts them in?):
branch.history().on('commit', function(cmt) { console.log(cmt.sha()); }).start()Maybe libgit2 just spews out commit objects in whatever order it wants?
Default sorting is "None", whatever that means. I've exposed sorting options:
https://github.com/nodegit/nodegit/blob/master/example/walk-history.js#L15
Note that this is now on master, not wip.
Oh, okay, so wip just became master and I should be using only master now?
Thanks for the quick response; excited to get back into my project now that I have a viable API!
Yes pls use master.
If I do a branch.history().on('commit', ...) and then within that, do a diffList.walk().on('delta', ...), I will pretty reliably get a "Segmentation fault: 11" for any git repository with more than 10 or 20 commits.
My solution has been to go through, first, and fetch all the commit shas, and then use caolan's async or something to iterate through those, using repo.commit('mysha123912', ...), and then walking the diffLists within each commit.
Is there a way to iterate through everything in one pass, though?
By "scatter memory", I mean that if I don't immediate call commit.sha(), but instead put the raw commit object in a queue, sometimes commit.message() and commit.sha() results won't line up correctly (they'll be contrary to what git log at the command line tells me).
(Great library, otherwise, loving the speed!)