After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 626702 - main loop source watching GAsyncQueue
main loop source watching GAsyncQueue
Status: RESOLVED DUPLICATE of bug 442364
Product: glib
Classification: Platform
Component: mainloop
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2010-08-12 03:55 UTC by Havoc Pennington
Modified: 2015-04-30 10:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
initial patch (11.93 KB, patch)
2010-08-12 03:55 UTC, Havoc Pennington
none Details | Review
naive finalize-source-outside-context-lock patch (1.39 KB, patch)
2010-08-12 15:16 UTC, Havoc Pennington
none Details | Review

Description Havoc Pennington 2010-08-12 03:55:33 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 1 Dan Winship 2010-08-12 13:28:58 UTC
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)?
Comment 2 Havoc Pennington 2010-08-12 14:16:26 UTC
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?
Comment 3 Havoc Pennington 2010-08-12 15:16:39 UTC
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.
Comment 4 Havoc Pennington 2010-08-12 16:27:35 UTC
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.
Comment 5 Havoc Pennington 2010-08-12 16:30:22 UTC
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 6 Dan Winship 2010-08-17 18:00:50 UTC
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 7 Dan Winship 2010-11-26 21:01:00 UTC
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
Comment 8 Matthias Clasen 2011-09-05 02:15:42 UTC
Compare with bug 442364
Comment 9 Allison Karlitskaya (desrt) 2011-09-10 00:52:30 UTC

*** This bug has been marked as a duplicate of bug 442364 ***
Comment 10 Tirpak Balazs 2015-04-30 10:25:08 UTC
(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