Repository navigation
Nested walks scatter memory and cause SEGFAULTS #72
Description
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!)
Activity
nkallen commentedon Jun 24, 2013
Can you please send us a coredump or run your code in gdb like and copy and paste for us?
chbrown commentedon Jun 24, 2013
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.
nkallen commentedon Jun 24, 2013
OK, one thing I'm pretty certain about:
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.
nkallen commentedon Jun 24, 2013
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 commentedon Jun 26, 2013
@chbrown I'd wait for @nkallen's code to come through, I believe his changes will solve this issue (and you'll have to rewrite some of your nodegit code anyway, might as well wait and do it all at once).
chbrown commentedon Jun 26, 2013
@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.nkallen commentedon Aug 11, 2013
If you're willing, please try the
wipbranch, which should fix this issue. Unfortunately there are many API changes for you to deal with though.nkallen commentedon Sep 5, 2013
Use
wipchbrown commentedon Sep 5, 2013
Works much better, no SEGFAULTs so far, thanks! I'm curious why the commits aren't in order (say, the order that
git logputs them in?):Maybe
libgit2just spews out commit objects in whatever order it wants?nkallen commentedon Sep 6, 2013
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.
chbrown commentedon Sep 6, 2013
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!
nkallen commentedon Sep 6, 2013
Yes pls use master.