Repository navigation
Threading model聽#827
Description
馃憢
I've been plumbing the depths of nodegit to understand how all the pieces fit together. I'm a bit concerned about the threading model.
My understanding:
- We enqueue asynchronous work with
Nan::AsyncQueueWorker. Nan::AsyncQueueWorkercallsuv_queue_work.uv_queue_workenqueues the work on a threadpool.
This means:
- We could do work on multiple threads at once.
- libgit2 says its objects shouldn't be used across multiple threads at once.
- We could end up accessing the same objects across multiple threads at once, most often the repository object.
- Potential 馃挜, definite violation of libgit2's threading model.
Is my understanding correct? Am I missing something?
Activity
srajko commentedon Dec 11, 2015
Heya,
I've also been looking into this. You are right about
Nan::AsyncQueueWorkerand that callinguv_queue_work, but afaict it queues the call on theuv_default_loop()(https://github.com/nodejs/nan/blob/master/nan.h#L1704), which is supposed to be the default node.js loop (https://nikhilm.github.io/uvbook/basics.html#default-loop). So I don't think it's using a threadpool, but I might be wrong.However, even if it is using an event loop on a single thread, this is only for NodeGit's async functions - the sync ones are presumably executing on a different thread, which would lead to the same potential violation of libgit2's threading model.
joshaber commentedon Dec 11, 2015
@srajko I think that's the loop that the
after_work_cbis delivered on, so that you end up back on the default event loop when you're done.But the
work_cbis called on the thread pool. I think. Which is what introduces the asynchrony. I think. The docs aren't terribly clear 馃槥joshaber commentedon Dec 11, 2015
Prior art: https://github.com/mapbox/node-sqlite3/blob/master/src/database.cc is more or less solving this same problem.
johnhaley81 commentedon Dec 11, 2015
@tbranyen we're talking about this in gitter and I think you should join if you have the time :)
joshaber commentedon Dec 11, 2015
For those following along at home, we talked on Slack and Gitter today and decided that this is indeed a problem. We talked about a lot of different solutions, but landed on having one thread per repository to serialize all access. If people need more asynchrony then they can open another repository connection.
@srajko has already started implementing this in #829. 馃
maxkorp commentedon Dec 11, 2015
That is a much less terrifying solution in scope than the others.
srajko commentedon Dec 17, 2015
Weird, I thought I posted an update about #829... well, the short of it is I got it to run all async functions on a dedicated thread, and mutex between that thread and the main sync thread. It still needs the thread/repo part. Ran into all kinds of weirdness using
uv_async_sendthough - a custom queue implementation might be better if we continue down this path.As an alternative I started #836. It uses the existing threading model (threadpool etc.) but allows us to lock objects when they are being passed to
libgit2. If we lock the repository associated with each object, the behavior is actually very similar to the thread per repo solution - a sync call can block the main thread if it's using the same repo as a running async function, but if you use separate repo objects you can get around the blocking (though in this case the number of concurrent async operations is limited by the threadpool).#836 is feeling a lot better this far into it (though here I have not yet tackled callbacks here - the ones going from libgit2 back to js, straddling both threads - and I am also not cleaning up the mutexes). It lets us specify exactly what needs to be locked based on the parameter type, be it the repo and/or something else (the prototype only describes what to lock for repo and index objects). The current implementation uses variadic templates - I am not sure if those are available in all targeted environments but I do hope so :-).
Thoughts?
joshaber commentedon Dec 17, 2015
I definitely like that 馃憤 I was a bit concerned about literally having a thread per repository previously. Thread pools ftw.
I could be misunderstanding you here, but couldn't we just always lock on the repository? I'm not sure what we'd gain by locking based on type, and it seems error-prone given that, e.g., a reference could touch its repository even if there isn't a repository in the parameters.
srajko commentedon Dec 17, 2015
In a lot of cases yes, but in some cases, we can't. For example, an index opened via http://www.nodegit.org/api/index/#open doesn't have an owner / repository, but we still need to prevent concurrent access.
I definitely agree with you - when possible, locking on the repository is probably the safest thing to do. The drawback is that concurrency suffers, so in some cases we could judiciously decide to not lock the whole repository (but yes, it is error prone).
For example -
git_commit. The following functions takegit_commitas a non-const pointer:git_cherrypick,git_cherrypick_commit,git_commit_free,git_commit_summary,git_revert, git_revert_commit. Out of these, all butgit_commit_freeandgit_commit_summaryalso take in a repository as a parameter. And for both of those, locking the commit is sufficient. So we could decide to only lock the commit - the advantage is that calls likegit_commit_summary,git_commit_messageetc. only block when that same commit is being used somewhere else, as opposed to blocking whenever the repo is being used. The risk is that locking the commit might not be sufficient - especially if the repo caches commits (and I think it does), and something else could be using this commit at the same time without explicitly locking it.So maybe we decide to just lock the commit, or we decide to lock the whole repo - but at least we have the flexibility (and we need the flexibility anyway to account for the no-repo case, as well as different ways of getting the repo out of the object depending on the type).
joshaber commentedon Dec 17, 2015
That all sounds fantastic. 馃憤 馃挴
srajko commentedon Dec 18, 2015
Added a few more things to #836. It now cleans up mutexes after they are used (probably too frequently - I would like to tie into something like the garbage collection cycle but I don't know how), and it locks multiple mutexes all at once (this should help prevent deadlocks).
I also made it so the thread safety is off by default and can be turned on (because this is a pretty substantial change, we may want to have it off by default at first).
This might be all the functionality needed for the first pass. I am going to start cleaning up and reorganizing code, try to make more of it opaque, get more test coverage and try to get it to build in more environments (hopefully keeping the C++11 features that I ended up using - mostly variadic templates and
shared_ptr... but we'll see).I might be a little slower with progress in the next week or so because of holidays but plan to keep moving this forward.
saper commentedon Dec 20, 2015
We can use
uv_async_*to queue async work in the main V8 loop. Using multiple threads in the pool is risky for one more reason: if those threads block for some operation, I/O operations from libuv will be stuck waiting for the async I/O worker to become available. Nasty examples from node-sass: sass/node-sass#857 sass/node-sass#1048johnhaley81 commentedon Dec 21, 2015
@saper I'm not quite following. If we queue with
uv_async_*and putting work on the JS loop aren't we actually blocking the app at that point by executing C++ in the JS thread unnecessarily? By putting work in the background we allow JS to stay responsive even during periods of blocking I/O. So even if something is taking a long time JS code will still get executed during this period.Right now we only put the minimum amount of work on the JS thread to get information down to the C/C++ layer or to handle the result from work. I don't really see why we should change that for async functions.
srajko commentedon Jan 6, 2016
If anyone wants to take a look at #836, I would love to get some feedback on it - I left more detailed comments on the PR. Thanks!
joshaber commentedon Jan 15, 2016
#836 landed and 0.8 includes it. Thanks for taking care of that @srajko!