GNOME Bugzilla – Bug 626702
main loop source watching GAsyncQueue
Last modified: 2015-04-30 10:25:08 UTC
Created attachment 167687 [details] [review] initial patch This patch implements g_async_queue_source_new() g_async_queue_add() g_async_queue_add_full() The idea is to allow a thread to queue items over to the main loop, which I've found to be a pretty common and useful thing to do. It's very little code to add to GAsyncQueue to support this, just a trivial GSource plus waking up main loop when items are pushed. To implement this in an app without the GAsyncQueue feature is some amount of work. One way is to essentially re-implement what I have here with a custom GSource, a mutex, and a GQueue. Another way is to have a GQueue+mutex (or GAsyncQueue), a G_DEFAULT_PRIORITY idle, and a mutex protecting whether the idle is queued. Yet another way is to use a GQueue+mutex (or GAsyncQueue), a notify pipe and an IO watch. Anyway g_async_queue_add() is much easier than any of these. The GCond in the async queue is not really used/useful if you have a main loop source (it only really makes sense to have the main loop consuming, or to have threads blocking on the condition consuming, I think, not both). However, GAsyncQueue still adds thread-safety (the mutex) and the item free func over plain GQueue. So I think it makes sense to have both modes of operation (main loop or blocking consumer thread) be part of GAsyncQueue. Possibly there's a scenario where the thread with the main loop can process queue items and also there are other blocking consumer threads that can also do it, but seems weird. Anyway it'd work if someone wanted to do it and that's the only time you'd need both the condition and the main loop source, I guess.
Comment on attachment 167687 [details] [review] initial patch This has the same deadlock bug 586432 had: GSource finalizers are run with the context's lock locked, so if you call g_async_queue_push() in one thread, it locks queue->mutex and then tries to grab context->mutex, but if the main thread has just run a GAsyncQueueSource that returned FALSE, it will have locked context->mutex and will then try to grab queue->mutex from g_async_queue_source_finalize(). Maybe we need to make GSource finalizers run outside the main context lock (like the prepare, check, and dispatch methods do)?
I can certainly look at ways to fix that, if this new API is interesting. Sounds logical to me that finalizers would be outside the context lock but I haven't dug into that code. A "heavy" solution of a separate mutex on queue->source is certainly possible but I hate to bloat things up with that. Dropping the queue->mutex before waking up the main context would be feasible except for the public g_async_queue_lock/unlock, and push_unlocked, where we couldn't do that; but maybe push_unlocked could document this caveat. We could also have a dedicated wakeup pipe instead of just relying on context_wakeup, so we'd just write a byte instead of touching the context from queue_push(), but again was trying to avoid the extra overhead. Anyhow, do people like the API, bugs aside?
Created attachment 167742 [details] [review] naive finalize-source-outside-context-lock patch This is the naive "just move source finalization out of the lock" patch. Surely there's some reason this won't work, but not apparent to me. Since the existing code already potentially drops the lock to invoke the closure dnotify callback, it seems like it already has to be expecting the lock to drop.
Owen points out that currently if you just queue a bunch of idles (one per item) they are guaranteed to be called in order and you don't necessarily need a queue at all; though it will be inefficient for a really large number of items. bug 619329 proposed potentially breaking that guarantee about ordering main loop sources but it turns out that would be a bad idea, we should probably document/test-suite the guarantee. Anyway I don't know whether the "bunch of idles" solution makes this API unnecessary or not. It does seem clunky somehow.
Incidentally bug 619329 does apply to the "bunch of idles" solution, it's O(n) to append to the queue if you're using main loop sources as queue items.
Comment on attachment 167742 [details] [review] naive finalize-source-outside-context-lock patch It looks right to me, but i'm not familiar with the dark internals of gmain. I just ran into another instance of this bug though: if you try to g_source_destroy() one source from inside the finalizer of another, it deadlocks, because the the inner call will try to acquire the lock on context, which the outer call already has, and it's not a recursive lock.
Comment on attachment 167742 [details] [review] naive finalize-source-outside-context-lock patch I committed a patch for the source finalization problem as part of bug 634239
Compare with bug 442364
*** This bug has been marked as a duplicate of bug 442364 ***
(In reply to Havoc Pennington from comment #3) > This is the naive "just move source finalization out of the lock" patch. > Surely there's some reason this won't work, but not apparent to me. (In reply to Dan Winship from comment #6) > Comment on attachment 167742 [details] [review] [review] > naive finalize-source-outside-context-lock patch > > It looks right to me, but i'm not familiar with the dark internals of gmain. And here is the reason why it did not work: https://bugzilla.gnome.org/show_bug.cgi?id=748642