[PATCH] Fix re-entrancy issues in wl_display_iterate().

[Pekka Paalanen]

On Sat, 14 Apr 2012 00:43:17 +0300
Hannu Lyytinen <hannu.lyytinen at nomovok.com> wrote:

> On Fri, 2012-04-13 at 11:49 +0300, Pekka Paalanen wrote:
> > On Fri, 13 Apr 2012 01:08:29 -0700 (PDT)
> > yan.wang at linux.intel.com wrote:
> > > wl_display_iterate will be called in multiply threads?
> > > Normally, I think it may be called in message loop thread.
> > > Thanks.
> > 
> > It does not seem to be about threads or parallelism, but nested
> > calls. wl_display_iterate() will dispatch events, therefore
> > call your handler functions, and if a handler function happens
> > to call wl_display_iterate(), you get screwed. This concerns
> > only wl_display_iterate(WL_DISPLAY_READABLE) type of calls.
> > 
> > Hannu, I am not sure allowing this is actually sane. Can you
> > clarify your case, why you need it?
> 
> Sorry, I cannot disclose full details (at least yet) because it
> appeared during a customer project. But it was basically a
> synchronous call over Wayland and we had to wait for the
> corresponding callback to be called. In other words, we called
> wl_display_iterate() only as long as we had not received the
> reply. And because we knew nothing can interfere or add anything
> extra into connection queue (we had written some kind of
> extension to weston compositor so we had control over what it
> does), it was deemed safe. The wl_display_iterate() would just
> munch the "return value". 
> 
> Then we had nested calls when the nested wl_display_iterate()
> caused problems. Even though we were 100% sure our nested call
> cannot alter the contents of the connection queue (in any other
> way than described above), the outer wl_display_iterate() didn't
> realize something some bytes were eaten from the queue and it
> tried to read past the end of the queue. This is what the patch
> fixes.

Does something prevent you from using the deferred task pattern
like upstream uses? By your explanation, it sounds like it would
achieve exactly the same (you have no other events coming when
you wait for your callback), and avoid a very fragile design.

If you did have other events coming in, they would get
dispatched in your current design, too, before your callback
comes. Maybe in the future you'd like to add the possiblity
to handle other events in the mean time to avoid being stuck?
Might not make sense right now, but...

My thought here is to try and discourage what I see as a fragile
design pattern, and I might even go as far as assert()'ing in
wl_display_iterate() if it is called nested. I'd love to hear
krh's ruling on this.

> So I would say nested wl_display_iterate() is OK as long as you
> really know what you are doing. You are right it cannot be used
> carelessly.

That a pretty big "if" IMHO, the implications are very complex,
and like you said, require special circumstances to be manageable
(at all?). You probably spent quite some time in trying to verify
it indeed is safe, and yet it backfired?


Thanks,
pq