Recursive dispatch (Re: [PATCH v2] server: Calculate remaining data size after a closure is processed)

[Jaeyoon Jung]

> -----Original Message-----
> From: Derek Foreman [mailto:derekf at osg.samsung.com]
> Sent: Wednesday, February 03, 2016 6:56 AM
> To: Pekka Paalanen; Jaeyoon Jung
> Cc: 'Jonas Ådahl'; wayland-devel at lists.freedesktop.org
> Subject: Re: Recursive dispatch (Re: [PATCH v2] server: Calculate remaining
> data size after a closure is processed)
> 
> On 02/02/16 06:57 AM, Pekka Paalanen wrote:
> > On Tue, 12 Jan 2016 13:03:19 +0900 "Jaeyoon Jung"
> > <jaeyoon.jung at lge.com> wrote:
> >
> >>> -----Original Message----- From: wayland-devel
> >>> [mailto:wayland-devel-bounces at lists.freedesktop.org] On Behalf
> >>> Of Jonas Adahl Sent: Tuesday, January 12, 2016 12:27 PM To:
> >>> Jaeyoon Jung Cc: wayland-devel at lists.freedesktop.org Subject:
> >>> Re: [PATCH v2] server: Calculate remaining data size after a
> >>> closure is processed
> >>>
> >>> On Tue, Jan 12, 2016 at 08:22:56AM +0900, Jaeyoon Jung wrote:
> >>>
> >>>> When processing a closure, data in the connection can be
> >>>> consumed again if the closure itself invokes extra event
> >>>> dispatch. In that case the remaining data size is also
> >>>> altered, so the variable len should be updated after the
> >>>> closure is processed.
> >>>>
> >>>> Signed-off-by: Jaeyoon Jung <jaeyoon.jung at lge.com>
> >>>
> >>> I don't like the name wl_connection_size
> >>> (wl_connection_pending_input() or something would be more
> >>> descriptive).
> >>
> >> I just wanted to have a shorter name.
> >> wl_connection_pending_input() looks better to me as well, and
> >> I'll update the patch accordingly. Thanks.
> >>
> >>
> >>> A good follow up would be a test case where this would
> >>> otherwise cause issues. I assume it would be triggered by a
> >>> wl_event_loop_dispatch() in a interface request handler?
> >>
> >> Yes, exactly.
> >
> > Hi,
> >
> > could you humor me and explain in what sort of situations
> > dispatching from within a request handler is useful?

Suppose a server has a single event queue for all kind of events
including wayland events. If the server wants to process more events
from the queue in the middle of the request handler, and there is
another wayland request arrived in the queue, it induces a recursive
dispatch. I'm not saying that this is a good practice though.


> > I thought recursive dispatch is a programming pattern that can
> > only lead to painful and erratic problems later. Don't we consider
> > this to be bad practice also in the client API?
> 
> Is that actually documented somewhere?
> 
> > If you do it in the server, that is like: this message causes more
> > messages to be dispatched - how does that make sense? A server must
> > not block waiting for any client message.
> 
> I agree that a server blocking on a client message is a great way to
> create a terrible user experience - but I'm not certain it's
> libwayland's mandate to prevent it?
> 
> > Also, inspecting future messages before finishing the processing of
> > the current message is racy: the following messages may or may not
> > be in the receive buffer and nothing guarantees either way, you
> > just get lucky a lot of times. Wayland IPC does not guarantee more
> > than one message to be delivered at a time, this is why we have
> > things like wl_surface.commit request.
> 
> This is a really good point. :)
> 
> > If someone starts hardening libwayland against programmer errors,
> > I'd expect one thing to be to abort() on recursive dispatch. Can
> > you tell me why we should support recursive dispatch?
> 
> IMHO we should either do that immediately (before the 1.10 release) or
> revert this patch now.  Not that anything is wrong with the patch - it
> doesn't introduce bugs in libwayland...
> 
> Previously recursive dispatch was impossible, with this patch it's
> possible.  The patch was landed to allow recursive dispatch to work,
> and it's obvious someone's about to start using it.

To be more exact, previously recursive dispatch allowed a message copy
from empty buffer and resulted in an irrelevant error post to the client.
If we want to disallow recursive dispatch, it would be nice to raise an
error like abort() when it is detected. Reverting this patch is just
going back to the past without resolving the issue.
I agree that recursive dispatch might be harmful and should be avoided.
However, I'm not sure if it is something must be done in libwayland.
Isn't it sufficient to be documented somewhere and let the implementor
make a decision?


> If we later intentionally break that code, we're jerks.
> -- From the jerk that posted a bunch of patches to do more stringent
> checks on resource versions a few days ago...  arguably those have
> always been bugs, though. ;)
> 
> So, with the release coming so quickly, I think we should either
> decide whether recursive dispatch is legal or illegal before the
> release, or revert the patch (thus re-breaking recursive dispatch) and
> worry about it for 1.11.
> 
> > I have nothing against this patch per se, mind. Just the whole
> > premise that is behind this patch smells fishy to me and makes me
> > think your software architecture that uses recursive dispatch is
> > fundamentally broken.
> >
> > Thanks, pq
> >


Best regards,
--
Jaeyoon (Jay) Jung (정재윤)