Kiid of like https://bugzilla.gnome.org/show_bug.cgi?id=639770

except for worker threads...

FWIW, tracker ran into the same problem a while ago

 http://permalink.gmane.org/gmane.comp.gnome.svn/528142

FWIW, I think the solution is to add a new flag to GDBusSignalFlags

 G_DBUS_SIGNAL_FLAGS_CALL_DESTROY_NOTIFY_IN_GDBUS_WORKER_THREAD

which will cause the GDestroyNotify functions (called when unsubscribing from the signal) in the GDBus worker thread instead of the thread-default GMainContext when registering. In the GDBusProxy case, it's this simple function

 http://git.gnome.org/browse/glib/tree/gio/gdbusproxy.c?id=2.35.1#n116

which only frees memory meaning that it's fine.

Created attachment 231516 [details] [review]
Patch

Here's a patch that does this. Milan, is it possible you can test it? Thanks!

(In reply to comment #3)
> FWIW, I think the solution is to add a new flag to GDBusSignalFlags
> 
>  G_DBUS_SIGNAL_FLAGS_CALL_DESTROY_NOTIFY_IN_GDBUS_WORKER_THREAD

The question I have is though - is GDBus the only thing of this form?  What about e.g. GSettings?

I think I like the "workaround" of scheduling an idle to quit the mainloop as someone mentioned, if it works.  That'd be pretty easy to enshrine as g_main_loop_quit_on_idle() or whatever.

But if this really is specific to the interaction of the GDBus worker thread and the app's thread, then maybe a GDBus flag is right.

(In reply to comment #5)
> (In reply to comment #3)
> > FWIW, I think the solution is to add a new flag to GDBusSignalFlags
> > 
> >  G_DBUS_SIGNAL_FLAGS_CALL_DESTROY_NOTIFY_IN_GDBUS_WORKER_THREAD
> 
> The question I have is though - is GDBus the only thing of this form?  What
> about e.g. GSettings?

I actually think GDBus is somewhat unique in running GDestroyNotify callbacks on the thread-default context (for GDBusConnection's signal_subscribe(), register_object() and register_subtree() methods at least). Other code seems not specify when notifiers are run, for better or worse. But in GDBus I ended up needing it.

> I think I like the "workaround" of scheduling an idle to quit the mainloop as
> someone mentioned, if it works.  

I think it will fail if one of the callbacks called after scheduling the idle, schedules another callback (if you know what I mean).

> That'd be pretty easy to enshrine as
> g_main_loop_quit_on_idle() or whatever.
> 
> But if this really is specific to the interaction of the GDBus worker thread
> and the app's thread, then maybe a GDBus flag is right.

It's a bit ugly, yea, but it solves a concrete problem so... but yeah.. anyway, since GDBusProxy is the only user, we could make the flag private and not expose it to user apps (use e.g. bit 31 or something).

(In reply to comment #6)
> 
> > I think I like the "workaround" of scheduling an idle to quit the mainloop as
> > someone mentioned, if it works.  
> 
> I think it will fail if one of the callbacks called after scheduling the idle,
> schedules another callback (if you know what I mean).
also falls over if there are any timeouts scheduled before the idle is scheduled.

The patch works for me. Here [1] is a scratch build with it for F18, in case anyone else is interested (until it's deleted).

[1] http://koji.fedoraproject.org/koji/taskinfo?taskID=4791508

I'll throw up a more general patch to implement the g_main_loop_quit_when_there_are_no_more_attached_sources I meantioned in the irc conversation.

Created attachment 231582 [details] [review]
mainloop: add API for quitting automitically

main loops are shared resources, so it's not always
easy to know when everyone is done using them and it's
safe to quit.

The main loop itself knows this information, though.

This commit adds new API for telling the main loop to
quit automatically when all sources are removed.

Note I'm not specifically proposing we do attachment 231582 [details] [review] instead of attachment 231516 [details] [review] .  We could do either, or both, depending on consensus.

I've wanted something like this a bunch of times when writing test cases. Especially when trying to make them properly-valgrindable; you need to make sure all pending callbacks have been run before finishing, since the callbacks may be holding refs on random objects.

Review of attachment 231582 [details] [review]:

I like the idea, dislike the name.  "_queue_quit()"?  "_quit_idle()"?

Those two names sort of imply that it's equivalent to

g_idle_add (g_main_loop_quit, loop)

(leaving aside g_idle_add doesn't use the loop's context, and g_main_loop_quit doesn't return FALSE);

but this does something different than that.  I mean, it's not "quit when no sources have pending events", it's "quit when there are no sources".

Also, another difference, is this is something that can be done up front before running the main loop, not just when the caller is ready to quit.

I'm not tied to _set_quit_automatically though. We could do "set_auto_quit", or "set_quit_on_no_sources", or "set_quit_when_unused" or "set_quit_after_use" or something else.

GDBus-specific solutions aside for a moment...

Would it not be possible for users of GMainContext in these short-lived-thread situations to ensure that they do:

  while (g_main_context_iteration (context, FALSE));

before freeing the GMainContext as a new "best practice"?

The only API I could imagine adding here is one that does the above as part of some sort of unref() or pop_thread_default() combined operation -- and I'm not sure we really need that...

that suffers from the same problem as walters g_main_loop_queue_quit proposal.  It doesn't account for a pending timeout.

g_main_context_flush()?

I could imagine a g_main_context_flush() call, but it would have to do more than flush pending events to completely solve the problem.  It would have to run until there are no more timeouts attached, so it would be more of a g_main_context_finish call than a g_main_context_flush call. Of course, in scenarios where g_main_context_finish() would be used, if it did work that way, there would be little need for a g_main_loop_run() call or even GMainLoop at all anymore.

Or maybe we don't care about the pending timeout case.  We call that more rare and not our problem. In which case, g_main_loop_queue_quit works, or g_main_context_flush or g_main_context_flush_pop_and_unref or whatever.

(In reply to comment #18)
> I could imagine a g_main_context_flush() call, but it would have to do more
> than flush pending events to completely solve the problem.  It would have to
> run until there are no more timeouts attached, so it would be more of a
> g_main_context_finish call than a g_main_context_flush call. 

I think these are all good points (and should definitely go into the docs for the method) but I don't see a good reason why it we can't call it flush(). After all - unless there's a compelling reason - we usually don't encode every little bit of semantic into the method name. For example, in this case, I don't think we're going to add other methods that are almost-similar-but-not-quite so I think we're safe.

(Btw, I also think that all GMainLoop instances attached to a flushing GMainContext would quit when flushing is done. But we don't need to encode that into the method name either - but we do need to document it in the docs.)

(In reply to comment #18)
> I could imagine a g_main_context_flush() call, but it would have to do more
> than flush pending events to completely solve the problem.

Yes, the intended meaning was "run until there are no more sources or pollfds attached". "g_main_loop_flush()" would probably make more sense. Although that doesn't sound as right...

I agree with Colin in not liking "automatic" though.

we could just add g_main_context_has_sources() and then people could loop running g_main_context_iterate() until g_main_context_has_sources() returns FALSE...

or "g_main_context_is_empty()"

> Or maybe we don't care about the pending timeout case.

we do

oh okay, I don't care what name we use as long as we can agree on the semantics and document it.

I talked to desrt, walters, and danw about this today on irc.

desrt pointed out that the main loop doesn't exactly know when it's safe to quit.  It only knows about attached sources, but a worker thread could plan to attach a source to the context later.  

The only way the main loop could know for sure that it's okay to quit is if the source list was empty and all outside refs to the loop's context were removed.  Quitting when the main loop's context goes away has a bit of a foul odor to it, though.  We would probably have to invent some new kind of "don't quit yet" reference or something. So that's hard and not obviously right.

Another alternative is to say the timeout case doesn't *really* matter, the future source from another thread case doesn't *really* matter, and the only case that matters is the queued idle case.  Solving that case might be just be good enough. 

Or we could punt entirely and say the programmer should know how the code it calls uses the main loop it provides (and urge libraries to document that).

I'm planning to push the patch in comment 4 since it's kinda orthogonal to the more general problem - if anybody has any strong objections to this, now would be a good time to voice them. Thanks!

Just for reference, the IRC conversation:

<desrt> halfline: can you explain the timeout usecase to me?
<desrt> (if you are awake yet) :)
<desrt> and in particular: do you propose that we dispatch timeouts immediately, or block and wait for them to fire?
<halfline> desrt: hey
<halfline> so the point is, a main loop is a shared resource
<halfline> and it could be the creator of the main loop doesn't know fully when other users of that main loop are done with it
<halfline> but the creator of the main loop is the one doing the quitting
<halfline> if the creator quits prematurely problems can crop up
<halfline> so my proposal was that the creator of the main loop shouldn't be the one that quits the main loop
<halfline> instead the main loop should quit when there are no users left
<halfline> (sources)
<halfline> i don't think timeouts should get blasted early
<halfline> i think the main loop should run until the timeouts are removed
<halfline> we put the onus on the code creating the main loop to be the one to quit the main loop, but don't provide any api to know when the loop can be safely quit
<halfline> we could provide that api, or we could remove the burden. my proposal argues for removing the burden
<desrt> halfline: so you're suggesting that the mainloop should control the life of the thread, effectively
<halfline> not the thread, the life of the loop
<halfline> the main loop should control its own life
<halfline> it knows when it's not needed anymore
<desrt> we're talking about the case of a worker thread, though, right?
<desrt> well... not worker, but not the main thread, either
<halfline> right, but the thread goes on after g_main_loop_run() finishes
<halfline> in fact, in the original case that spurred the bug
<desrt> you're effectively asking that g_main_loop_quit() stop existing
<halfline> the problem was because of gdbus attaching a source after g_main_loop_run finished
<desrt> which, as the author of GApplication, i have to admit i understand the appeal of...
<walters> were we to start again, i'd probably have made g_main_loop_quit() have those semantics
<halfline> well my proposal puts a boolean on the loop object
<halfline> if the boolean is true, g_main_loop_quit shouldn't rarely get used
<halfline> if the boolean is false, g_main_loop_quit still needs to get used
<halfline> s/shouldn't rarely/should rarely/
<halfline> basically you tell the loop whether or not it manages itself
<desrt> i'm not happy
<halfline> and of course even if it is managing itself, g_main_loop_quit would still work
<desrt> consider this...
<desrt> i push the default main context in a thread
<desrt> do an async call
<desrt> the async result object for the call take a ref on the context
<desrt> i pop it
<desrt> there are no sources on the loop
<desrt> i want to quit....
<desrt> but meanwhile there is this pending event that just happens not to be a GSource yet
<desrt> that kind of separation seems a bit arbitrary to me
<desrt> ie: what we'd really want is to have the mainloop run for as long as a ref is held on the context -- and that's just insane
<desrt> unless we define some new sort of ref for contexts that says "i will be adding a callback here soon, maybe"
<desrt> and now we're getting really close to g_application_hold()....
<halfline> can you explain the "pending even that just happens not to be a GSource yet" in greater detail for me?
<desrt> the usual way that async calls for something like GDBus work is like so:
<desrt> i have a thread-default main context that's pushed to a TLS pointer -- it gets read at the time of the _async() call and a ref is held on that context
<desrt> usually by the GAsyncResult object that will eventually be used to report the completion (by convention we typically create GSimpleAsyncResult/GTask at the time of the async call and attach the context there)
<desrt> so now i have a ref on a context but absolutely nothing else
<desrt> this async call is exactly the same as a timeout, really: in the future, the main context will dispatch a callback
<desrt> so then the async call becomes finished
<desrt> what happens then is that we schedule an idle on the main context in order to report it
<desrt> the main context that was the thread-default one at the time the call was made...
<halfline> oh i see where you're going with this
<desrt> that main context could be 'dangling'
<desrt> ie: the only ref held on it at that point may be the one that was held by the async result state object
<desrt> so we'll schedule the idle into the context and unref it -- destroying it
<desrt> meanwhile the loop will have 'automatically' quit long ago for lack of having any actual sources in it
<halfline> hmm
<desrt> i really think we either need to have a much larger hammer here or we need to just tell programmers that we have higher expentations of them
<desrt> ie: if there may be something "pending" on the context (in the sense of an actual source, or a potential future source, as above) then they are responsible for continuing to run it
<halfline> yea i think i agree with you
<walters> desrt: in this case though, the app programmer knows when all async calls they've made are complete; it'd be unexpected if there was an async op queued internally by glib outstanding when all ops they'd started had completed
<desrt> walters: could easily be the case that some library has queued an 'internal' async call on their context, though...
<desrt> we do that sort of thing in libraries all the time....
<halfline> though if we expect more from the programmer, we need to expect more from the provided documentation too
<desrt> halfline: the other alternative is that we get even more pragmatic
<desrt> and we realise that idles really are special...
<desrt> ie: stop trying to solve the problem for timeouts
<desrt> afaik there's no "real problem" here that involves timeouts
<halfline> yea i meantioned that as a possibility on the bug report
<desrt> so that goes back to my suggestion of doing a while(iteration(false))
<halfline> danw was pretty adamant we solve the timeout case too
<desrt> and maybe having some API based on that
<desrt> i don't agree with that
<desrt> timeouts are even less likely to be a real problem than pending async callbacks
<halfline> well he didn't give his reasons, so maybe he just hasn't persuaded you yet. :-)
<halfline> danw: around?
<danw> yes
<halfline> danw: can you read scrollback and chime in with your thoughts ?
<walters> maybe async ops should hold a weak ref on the context - if it gets destroyed, the callbacks get moved to the glib worker thread
<desrt> walters: no way dude
<walters> yeah...it's kinda crazy
<desrt> gotta fly
<desrt> will talk later
<halfline> oh ok
<danw> halfline, desrt: i wouldn't say i was "adamant" about timeouts, it just seemed arbitrary to me to make it work for some sources but not others
<danw> but desrt's point about async callbacks is a good one. there might not be any sane API that has the semantics I want. And I don't remember the detailed specifics of what I was up to when I was doing these things

Probably too late for 2.36 on this one now, but...

(In reply to comment #24)
> <desrt> consider this...
> <desrt> i push the default main context in a thread
> <desrt> do an async call
> <desrt> the async result object for the call take a ref on the context
> <desrt> i pop it
> <desrt> there are no sources on the loop
> <desrt> i want to quit....
> <desrt> but meanwhile there is this pending event that just happens not to be a
> GSource yet
> <desrt> that kind of separation seems a bit arbitrary to me
> <desrt> ie: what we'd really want is to have the mainloop run for as long as a
> ref is held on the context -- and that's just insane
> <desrt> unless we define some new sort of ref for contexts that says "i will be
> adding a callback here soon, maybe"

Or we could reorganize how GTask and similar APIs work. It could attach the completion source to the GMainContext right away (ie, in g_task_new()), but then call g_source_set_ready_time(-1). Then when it was ready to dispatch it, call g_source_set_ready_time(0).

Then a "wait until all sources have been removed from the context" API could work again.

This is WONTFIX territory for me... I'm not sure it's worth expending that much effort.

FWIW, we hit a bug in GJS where this would have been useful, bug 697436.

Maybe we could add the g_main_context_has_sources() call Dan proposed in comment 20. That callers can provide "good enough" solutions for their own uses even if it's not sufficient to give a fool proof quit-when-ready api (that could be put in glib proper).

interesting side discussion:

<desrt> i consider GMainLoop semi-deprecated
<walters> woah wha?
<desrt> seriously
<walters> what replaces it?
<desrt> i don't think it's a very good thing to use for... almost anything, really
<desrt> patterns like i did in this patch
<desrt> GApplication has some similar logic in side of it
<desrt> while (use_count) g_main_context_iterate();
<walters> interesting
<desrt> the idea of having some weird object that iterates a maincontext until you quit() it from somewhere else is weird and tends to cause bugs like this one...
<walters> i am unable to fit the possible ramifications of this in my mental model of application programming at the moment
<walters> i definitely like a lot how it makes it easy to encode the termination condition in exactly one place
<walters> that being the part i keep screwing up
<desrt> it also is less code....