nodegit crash when trying to use Submodule.foreach

[tylerchurch]

I was trying to use nodegit to do a recursive clone, but was getting crashes left and right and couldn't figure out what was going on. I've found a workaround, but wanted to report the crash here.

As far as I can tell, if I used Submodule.foreach to iterate over the submodules and then call submodule.update, nodegit crashes the node process, and I get an error message popup that looks like this:

Microsoft Visual C++ Runtime Library
Assertion failed!

Program: ...re\node_modules\nodegit\build\Release\nodegit.node
File: ..\..\vendor\libgit2\src\repository.c
Line: 2111

Expression: repo && id

For information on how your program can cause an assertion failure, see the Visual C++ documentation on asserts.

( Here's a link to an image of the screenshot: https://dl.dropboxusercontent.com/u/108847970/nodegit-crash-screenshot.png )

These's are the versions I'm running:

• Windows 10 (x64)
• nodejs 4.2.3 (x64)
• nodegit 0.11.5

Here's some sample code that reproduces the issue for me:

var path = require("path");

var mkdirp = require("mkdirp");
var rimraf = require("rimraf");
var git = require("nodegit");
var reposPath = path.join(__dirname, "cloned-repos");
var repoPath = path.join(reposPath, "electron");
var cloneUrl = "https://github.com/atom/electron";

rimraf(reposPath, function (err) {
    if (err) {
        throw err;
    }
    mkdirp(reposPath, function (err) {
        if (err) {
            throw err;
        }

        console.log("Cloning '" + cloneUrl + "'...");

        git.Clone.clone(cloneUrl, repoPath, new git.CloneOptions()).then(function () {
            console.log("Cloned successfully!");

            git.Repository.open(repoPath)
                .then(function(repo) {
                    var p = Promise.resolve();

                    git.Submodule.foreach(repo, function(submodule) {
                        var name = submodule.name();
                        console.log("Found submodule: " + name);

                        p = p.then(
                            function() {
                                return new Promise(function(resolve, reject) {
                                    submodule.init(1).then(
                                        function() {
                                            console.log("Init success on " + name);

                                            submodule.update(1, new git.SubmoduleUpdateOptions()).then(
                                                function () {
                                                    console.log("Update success on " + name);
                                                    resolve();
                                                },
                                                reject
                                            );
                                        }, reject
                                    );
                                });
                            }
                        )
                    }).then(
                        function() {
                            console.log("Foreach is done.");

                            p.then(
                                function() {
                                    console.log("Everything is done.");
                                },
                                function(error) {
                                    console.error(error);
                                }
                            )
                        }
                    );
                });
        }, function (error) {
            console.error(error);
        });
    });
});

The workaround I discovered was that if I switch to using Repository.getSubmoduleNames and Submodule.lookup instead of Submodule.foreach, everything seems to work just fine.

Here's the sample code for that:

var path = require("path");

var mkdirp = require("mkdirp");
var rimraf = require("rimraf");
var git = require("nodegit");
var reposPath = path.join(__dirname, "cloned-repos");
var repoPath = path.join(reposPath, "electron");
var cloneUrl = "https://github.com/atom/electron";

rimraf(reposPath, function (err) {
    if (err) {
        throw err;
    }
    mkdirp(reposPath, function (err) {
        if (err) {
            throw err;
        }

        console.log("Cloning '" + cloneUrl + "'...");

        git.Clone.clone(cloneUrl, repoPath, new git.CloneOptions()).then(function () {
            console.log("Cloned successfully!");

            git.Repository.open(repoPath)
                .then(function(repo) {

                    repo.getSubmoduleNames().then(
                        function(names) {
                            console.log("Found the following submodules:");
                            console.log(names);

                            var p = Promise.resolve();

                            names.forEach(function(name) {
                                p = p.then(
                                    function() {
                                        return new Promise(function(resolve, reject) {
                                            git.Submodule.lookup(repo, name).then(
                                                function (submodule) {
                                                    submodule.init(1).then(
                                                        function() {
                                                            console.log("Init success on " + name);

                                                            submodule.update(1, new git.SubmoduleUpdateOptions()).then(
                                                                function () {
                                                                    console.log("Update success on " + name);
                                                                    resolve();
                                                                },
                                                                reject
                                                            );
                                                        }, reject
                                                    );
                                                }, reject
                                            );
                                        });
                                    }
                                )
                            });

                            return p;
                        }
                    ).then(
                        function() {
                            console.log("Done!");
                        },
                        function(error) {
                            console.error(error);
                        }
                    );
                });
        }, function (error) {
            console.error(error);
        });
    });
});

A zip of the files used in the above examples (including my compiled version of nodegit) is available here if that helps at all: https://dl.dropboxusercontent.com/u/108847970/nodegit-foreach-failure.zip

I had screen other crashes in the past when I was using Submodule.foreach as well, but I had never been able to identify the cause as being that specific function. Now I think that it was.

I would try to track down this issue to the source myself, but I have no idea where to even start on debugging this kind of thing 😛

Let me know if I can provide any other additional information.

[maxkorp]

Before you call submodule.update, can you call submodule.reload(1)?

[tylerchurch]

You mean changing the above to this?:

return new Promise(function(resolve, reject) {
    submodule.init(1).then(
        function() {
            console.log("Init success on " + name);
            submodule.reload(1);
            submodule.update(1, new git.SubmoduleUpdateOptions()).then(
                function () {
                    console.log("Update success on " + name);
                    resolve();
                },
                reject
            );
        }, reject
    );
});

Same error.

[maxkorp]

Correct

[michaelmcallister]

I'm running into the same problem - and calling submodule.reload(1) didn't change anything for me, I rewrote @tylerchurch's code to suit my needs which just git clones each submodule, it works okay, depending on what you need.

var git = require("nodegit");
var repo_path = "folder";
var clone_url = "https://github.com/user/repo";

git.Clone.clone(clone_url, repo_path, new git.CloneOptions()).then(function () {
    console.log("successfully cloned");

    git.Repository.open(repo_path).then(function(repo) {
        git.Submodule.foreach(repo, function(submodule) {
            var sub_name = submodule.name();
            var sub_url = submodule.url();
            var sub_path = repo_path + "/" + submodule.path();

            console.log("found submodule: %s at %s", sub_name, sub_url);

            git.Clone.clone(sub_url, sub_path, new git.CloneOptions()).then(function () {
                console.log("successfully cloned %s into %s", sub_name, sub_path);
            });
        });
    });
}, function(err){
    console.log("Fatal %s", err);
});

[rcjsuen]

I tried the example @tylerchurch provided and I did get the same segmentation fault he did. On another run, the program died with an Odb read failure.

Cloning 'https://github.com/atom/electron'...
Cloned successfully!
Found submodule: vendor/boto
Found submodule: vendor/breakpad
Found submodule: vendor/brightray
Init success on vendor/boto
Found submodule: vendor/crashpad
Found submodule: vendor/depot_tools
Found submodule: vendor/native_mate
Found submodule: vendor/node
Found submodule: vendor/requests
Foreach is done.
Failed here:    Error: Object not found - no match for id (6566732f72656d6f7465732f6f726967696e2f7)

Not surprisingly, the object cannot be found in the repository.

...cloned-repos\electron> git cat-file -p 6566732f72656d6f7465732f6f726967696e2f7
fatal: Not a valid object name 6566732f72656d6f7465732f6f726967696e2f7

In fact, if you look closely, it is actually a 39-character hash instead of a 40-character hash!

[rcjsuen]

Here are two different failure cases that are caused by assertion failures. The first one is the original problem reported in this bug. I think the second is probably just an unfortunate by-product of the same problem.

detach(git_repository * repo, const git_oid * id, const char * from) Line 2126	C
git_submodule_update(git_submodule * sm, int init, git_submodule_update_options * _update_options) Line 1165	C
GitSubmodule::UpdateWorker::Execute() Line 2977	C++
ThreadPool::RunEventQueue() Line 49	C++
ThreadPool::RunEventQueue(void * threadPool) Line 32	C++

git_submodule__open(git_repository * * subrepo, git_submodule * sm, unsigned char bare) Line 1301	C
git_submodule_update(git_submodule * sm, int init, git_submodule_update_options * _update_options) Line 1176	C
GitSubmodule::UpdateWorker::Execute() Line 2977	C++
ThreadPool::RunEventQueue() Line 49	C++
ThreadPool::RunEventQueue(void * threadPool) Line 32	C++

The line numbers aren't perfect (I'm not sure what happened with Visual Studio) but you get the idea.

[rcjsuen]

OK, I think I figured it out. Of course, the solution was right before my very eyes.

In git_submodule_foreach, each submodule in the git_vector is freed(!) after git_vector_foreach is executed on the submodules. This would explain why the submodules could no longer be used "in the future" (per promise resolution) and that simple sanity assertions in libgit2 was failing and causing Node to crash.

I have run the test code several times but vendor/boto would always complete in the midst of the git_submodule_foreach call

Found submodule: vendor/boto
Found submodule: vendor/breakpad
Found submodule: vendor/brightray
Init success on vendor/boto
Found submodule: vendor/crashpad
Found submodule: vendor/depot_tools
Found submodule: vendor/native_mate
Found submodule: vendor/node
Found submodule: vendor/requests

This seems believable as it's the very first submodule that is encountered so it seems that its promise gets a chance to resolve in the middle of the git_submodule_foreach call. I guess between the callbacks to the various submodules back to the JavaScript side from C the JavaScript thread relinquishes some CPU cycles for promise resolution...?

Anyway, I'm not sure what the NodeGit team wants to do here. I would probably just consider rewriting an implementation of git_submodule_foreach in JavaScript that basically just does what @tylerchurch with the help of git_submodule_lookup. It might not be performant (maybe it's actually fine, I don't know) but it seems like it would satisfy the simple use case of iterating over all the submodules of a repository.

[sgraf812]

You can always first iterate through all submodules with foreach to store the submodule name and then in a second pass (don't forget to await/chain with then) iterate through all submodule names, look them up with #lookup and #update them. It's horrible, but will work*.

* As long as you don't have a submodule pointing to a redirected URL, as I just found out. Yuck, libgit2 is the worst.

[implausible]

It's another thing for the laundry list of memory management in nodegit... I imagine we might want a dirty bit on our NodeGitWrapper so that in cases like this we can dirty the submodules on for each exit so they they are considered unusable.

[sgraf812]

That still won't help with the race condition (lifetime of one foreach step vs. lifetime of the update promise spawned in each step), though.

[implausible]

If you return the promise in the callback, it should wait for the promise to resolve before exiting the git_submodule_foreach. Callbacks have a built in promise resolver in nodegit. If you are calling a git_submodule_foreach in a fire and foreget pattern, that would be a problem though.

It does look like in nodegit the lifetime of a git_submodule in a git_submodule_foreach is until the end of the git_submodule_foreach call. Correct me if I'm wrong.

[sgraf812]

If you return the promise in the callback, it should wait for the promise to resolve before exiting the git_submodule_foreach. Callbacks have a built in promise resolver in nodegit. If you are calling a git_submodule_foreach in a fire and foreget pattern, that would be a problem though.

Interesting, I didn't know that and wondered how others would get away with this.

It does look like in nodegit the lifetime of a git_submodule in a git_submodule_foreach is until the end of the git_submodule_foreach call. Correct me if I'm wrong.

That's not what I observed. This snippet:

    var my_clone_path = this.clone_path;
    await Git.Submodule.foreach(repo, function(submodule: Git.Submodule) {
            var sub_name = submodule.name();
            var sub_url = submodule.url();
            var sub_path = my_clone_path + "/" + submodule.path();

            console.log("found submodule: %s at %s", sub_name, sub_url);
            return submodule.update(1, null);
        }, null);

will crash upon trying to update the submodule. This contradicts either your first or second paragraph.