Re: Changes to the GLib main loop [ remainder ]

[Owen Taylor <otaylor redhat com>]

> > A final general consideration with your changes is that you
> > have effectively made the public functions:
> > 
> >   gboolean g_main_context_prepare  (GMainContext *context,
> >                                     gint         *priority);
> >   gint     g_main_context_query    (GMainContext *context,
> >                                     gint          max_priority,
> >                                     gint         *timeout,
> >                                     GPollFD      *fds,
> >                                     gint          n_fds);
> >   gint     g_main_context_check    (GMainContext *context,
> >                                     gint          max_priority,
> >                                     GPollFD      *fds,
> >                                     gint          n_fds);
> >   void     g_main_context_dispatch (GMainContext *context);
> > 
> > Not usuable, because g_main_context_iterate() manipulates
> > the context structure directly in ways that external code
> > can't. If you look at the current implementation of
> > g_main_context_iterate(), you'll see that the only direct
> > use of the GMainContext structure is to cache the GPollFD
> > array.
> 
> No, that's a bogus argument, I didn't change those functions. They are as
> usable as before and still you can easily program g_main_context_iterate as in
> gmain.c. You only would need to hold the owner, cond and need_broadcast
> members in your own structure, as before for cached_poll_array and
> cached_poll_array_size. Of course mixing own mainloops with the GLib one is a
> delicate thing, but that has been the case with the old solution as well.

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.

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.

[...]
 
> > > - g_main_context_iterate handles cached_poll_array more inteligent.
> > >   The old solution looked quite doubios to me.
> > 
> > I'm not sure your handling is "more intelligent". It is certainly
> > simpler. My initial reasoning for the complexity was that the
> > fd array needed to remain the same over the entire
> > prepare/query/check() sequence; the code was meant to ensure that.
> 
> What I mean with doubios is, that whenever you loop through the do {} while
> loop for the second time, you have a memory leak, so the while (new_nfds >
> nfds) should rather be a g_assert (new_nfds > nfds), but that could fail, as
> you do not have the lock, while calling g_main_context_query, and add_poll
> could intercept the whole thing.

Well, OK that code is buggy ;-). But no, it shouldn't have been an assert(),
rather it should have simply had a:

 if (fds)
   g_free (fds)

At the top of the loop. I wouldn't have put it in a loop if I didn't 
expect it to loop sometimes. Anyways, I think your simpler change
probably works OK.
 
> > 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.

> 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.

[...]
 
> > > Another thing, that really makes me wonder are the lines
> > >
> > >   'if (pollrec->fd->events)'
> > >
> > > in g_main_context_qurey and g_main_context_check. Can't this be
> > > checked in g_main_context_add_poll, or do we allow fd's with empty
> > > event fields to be added. Actually this would indicate a programming
> > > mistake, doesn't it?
> > 
> > I think it is mainly a case of allowing all cases, including the
> > NULL one for the sake of completeness. I'm not sure a 0 field
> > here is actually a programming mistake... it could occur
> > theoretically through simple laziness.
> 
> 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.

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.

   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.

 * GMainLoop needs a condition variable per loop.

Regards,
                                        Owen