Threading model #827
Description
Activity
Heya,
I've also been looking into this. You are right about Nan::AsyncQueueWorker and that calling uv_queue_work, but afaict it queues the call on the uv_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.
@srajko I think that's the loop that the after_work_cb is delivered on, so that you end up back on the default event loop when you're done.
But the work_cb is called on the thread pool. I think. Which is what introduces the asynchrony. I think. The docs aren't terribly clear 😞
Prior art: https://github.com/mapbox/node-sqlite3/blob/master/src/database.cc is more or less solving this same problem.
@tbranyen we're talking about this in gitter and I think you should join if you have the time :)
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.
That is a much less terrifying solution in scope than the others.
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_send though - 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?
It uses the existing threading model (threadpool etc.)
I definitely like that 👍 I was a bit concerned about literally having a thread per repository previously. Thread pools ftw.
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).
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.
I could be misunderstanding you here, but couldn't we just always lock on the repository?
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'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.
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 take git_commit as a non-const pointer: git_cherrypick,git_cherrypick_commit,git_commit_free,git_commit_summary,git_revert, git_revert_commit. Out of these, all but git_commit_free and git_commit_summary also 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 like git_commit_summary, git_commit_message etc. 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).
That all sounds fantastic. 👍 💯
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.
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#1048
@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.
👋
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:
This means:
Is my understanding correct? Am I missing something?