I'm at a stopping point with this implementation. The main part is the LockMaster class which holds locks related to parameters being passed to libgit2. Currently, when we pass a repository object, we lock the repository. When we pass an index, we lock the repository that owns the index (or the index, if there is no owner). When we pass a commit, we lock the repository that owns the commit. Support for more types can be easily added.

The objects are locked when the LockMaster is instantiated, and unlocked when it's destroyed. Another class, LockMaster::TemporaryUnlock, is used to temporarily unlock these objects when the native layer in an async thread needs to process a callback in the javascript layer (otherwise a call from inside the callback to a sync method that needs to lock one of the locked objects would deadlock the system).

The current implementation uses a few c++11 features, like variadic templates, range-based loops and auto. All of these can be replaced with non-c++11 code, if needed, but I think the library should still build on the compilers we are targeting.

The thread safety system can be disabled / enabled, and it is currently disabled by default.

It would be great to get some feedback on the implementation, also trying to build on different systems would be helpful.

This does compile on gcc 4.6 and clang 3.4

Wow, this is amazing!

The thread safety system can be disabled / enabled, and it is currently disabled by default.

I'd tentatively argue is should be safe (enabled) by default.

My C++ is pretty rusty, but I didn't see any obvious problems 👍

@joshaber thanks for taking a look! I think you are right about having thread safety enabled by default, but maybe we could ease into it (have it disabled by default for a release or few, then turn it on). My main concern is that turning it on makes it possible to block the main node thread, so existing code using NodeGit can be adversely affected (and if it wasn't running into thread-safety issues, it would be adversely affected with no benefit... though scenarios in which the main thread would get blocked are likely the scenarios in which thread safety is an issue, so maybe my concern is unwarranted).

It would be nice if in the long term, we find a way to avoid the risk of blocking the main thread. That would mean that sync functions would never lock - and we've already discussed a few ways of achieving that (make sure that sync calls read data in a thread safe way, make everything else async, etc.), so maybe it wouldn't be too hard to get to that point.

Do we want to have tests running for both thread safety enabled and disabled? Currently they run with the default (disabled) except for index tests, where they are explicitly turned on to test a deadlock scenario.

If we want both, what would be a good way to implement that?

I think you are right about having thread safety enabled by default, but maybe we could ease into it (have it disabled by default for a release or few, then turn it on).

Yeah, that's a fair concern too. I'm not sure how to best communicate this change. /cc @nodegit/owners

If we want both, what would be a good way to implement that?

Perhaps an environment variable, so that it can be part of the build matrix?

eh, not a bit fan of the both option, as then there are 2 modules to install. I'd say get through like 0.8.x with it disabled by default, and then a bump to 0.9.x with it enabled by default

I'd say make it default when we go to 1.0

Yeah thats a good idea, that way anybody has to explicitly upgrade to it in a project if htey've got stupid ^ deps in their package.json

I agree with switching on by default in 1.0, even though thread safety is more of a guideline than a rule /s

Just looked over this PR and honestly I think it's ready. Since it defaults to off I feel like this is a moderately safe PR to pull in and we'll be flipping thread safety on in GitKraken so we'll be doing some real-life testing and we'll iron out bugs as we go. I'm removing the [WIP] from this and if nobody speaks up I'll merge this in in about an hour.

@joshaber the environment variable idea sounds great to me. @maxkorp I don't think this would require two versions of the module (though you might be thinking back to when I was proposing to add a switch to build NodeGit with/without the thread support).

The way I am interpreting this is we would have an environment variable, maybe NODEGIT_TEST_THREADSAFETY, and if it's set the tests would (at run-time) turn on thread safety. By default, they would test with thread safety off.

I'd like to add this anyway, if nothing else to make it easier to test the thread safety code locally. I'll open up a separate PR when I get a chance.