Re: Changes to the GLib main loop [ remainder ]

[Sebastian Wilhelmi <wilhelmi ira uka de>]

And again a salute,

> Before your changes:
> 
>  * You could only call g_main_context_iteration() from a single thread,
>    but you could call g_main_loop_run() from other threads.
> 
>  * When doing that, you could use any combination of g_main_loop_run(),
>    g_main_context_iteration(), and a custom main loop written
>    using the primitive functions, and it would work fine.
> 
> Now:
> 
>  * You can call g_main_context_iteration() from multiple threads as long
>    as you _only_ use g_main_context_iteration().
> 
>  * If you use a custom main loop written from the primitives, you are
>    restricted to calling g_main_context_iteration() and
>    g_main_loop_run() from a single thread, with no warnings at all if
>    you violate this constraint.
> 
> What I'm uncomfortable with is that you've added a concept of the
> "owner" of a main loop, and you require this owner to be set
> for proper operation, but you haven't exposed this concept to
> people writing custom main loops.

Ok, now I see what you mean: Using the GLib main loop and custom mainloops
concurrently wasn't a problem before, but is now.

> I guess one way of handling this would be to simply _expose_ the
> concept of the owner.
> 
>  /* Try to become owner of a context, return TRUE on success.
>   * (properly recursive)
>   */
>  gboolean g_main_context_try_own  (GMainContext *context);
> 
>  /* Disown a context previously acquired by g_main_context_own()
>   */
>  void g_main_context_disown   (GMainContext *context);
> 
> That's enough for safety, though it doesn't allow the full capabilities
> of g_main_context_iterate(), g_main_loop_run() to be reimplemented.

That would work, we would only have to replace reset_owner by a owning_count
in the context. Then indeed it should work fine, doing the g_cond_broadcast in
g_main_context_disown. We would only need a g_main_context_wait function that
would just call

context->need_broadcast++;
g_cond_wait (context->cond, g_static_mutex_get_mutex (&context->mutex));
context->need_broadcast--;

Then user programs could easily integrate the GLib main loop into their own.

> > > From a brief look, it appears that since g_main_iterate() can
> > > never recurse until the dispatch() phase, your change is probably
> > > OK, assumming you fix up g_main_context_add_poll().
> > >
> > > > - GStaticMutex is used instead of GMutex. This makes it possible
> > > >   to initialize a GMainContext before calling g_thread_init ();
> > >
> > > Didn't we say that g_thread_init() _must_ be called before any
> > > other GLib functions. Using a GStaticMutext for this reason
> > > strikes me as a bit dubious...
> >
> > Until now we didn't require this, on the contrary we made sure to let
> > the main thread inherit all thread private data set before
> > g_thread_init () and so on. In general some libraries might to
> > initialize the thread system, which the main program doen't know about
> > or something like that, so I think, that requiring this isn't good.
> 
> Hmm, I guess considering my complaints about g_type_init(), I can't
> really object to this.
> 
> But what does worry me is:
> 
>  Library A is initialized in non-threaded mode
>  Library B is initialized, calls g_thread_init()
>    Library B makes calls to library A, which isn't in threaded mode,
>    from a second thread.
> 
> It could even happen that it might not work to make calls to
> Library A from a single thread before and after g_thread_init() is
> called - it might do something like I was doing with GMainContext.

Yes, but that should not make GLib impose unnessecary restrictions, I think.

> > Also you save a malloc, and an indirection, as the mutex is in situ.
> 
> These reasons I don't believe :-)
> 
>  * GMainContext is not meant to be a lightweight structure
>  * You don't save an indirection - either pthread_mutex_lock() gets
>    a pointer to a mutex.

Ok, I withdraw that arguments. typist, please strike that out of the files ;-)

> > > >   'if (pollrec->fd->events)'

> > I would still sort it out in g_main_context_add_poll, not nessecarily
> > with g_return_if_fail, but why not, actually?
> 
> I don't think its a big thing either way - yes it could be done
> in g_main_context_add_poll(), though we'd have to make sure
> that g_main_context_remove_poll() accepted GPollFD * that weren't
> in the poll array.
> 
> And you'd have to be careful to do:
> 
>   fd->revents = 0;
> 
> In g_main_context_add_poll() even when not actually adding the
> new GPollFD.

Ok, that is an unrelated thing, I'll come back to that, when we got over the
rest.

> So, I guess, to summarize the big isses from my responses:
> 
>  * I don't think the ->owner scheme makes iterating from multiple
>    threads actually useful, but it may work better in other
>    ways than having a static owner.

Agreed, I never intented to make iterating from multiple threads useful, it
was just to make g_main_context_iterate behave more consistent in addition to
the original aims:

 1) making it possible to have more than one context per thread

 2) adding sources to contexts before the thread for the context is there.

>    I don't have a good sense of how to handle setting the owner
>    from custom main loops, but I think it is an important
>    issue to address.

I think, your proposal (with the additional g_main_context_wait) should
indeed work, in fact we could use the functions try_own, disown, wait (or
maybe call them acquire, release and wait) for our own implementation, thus
making it even more evident, that outside programs could use that system
themselfs.

>  * GMainLoop needs a condition variable per loop.

As stated in the other mail, I think, that's a bad idea. Of course you get
spurious wakeups for different main loops, that use the same context, but this
really is a very fast operation compared to the iteration itself, and it won't
happen too often.

Bye,
Sebastian
-- 
Sebastian Wilhelmi
mailto:wilhelmi ira uka de
http://goethe.ira.uka.de/~wilhelmi