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

[Auke Booij]

On 3 February 2016 at 09:34, Jonas Ådahl <jadahl at gmail.com> wrote:
> On Wed, Feb 03, 2016 at 11:14:54AM +0200, Pekka Paalanen wrote:
>> On Wed, 3 Feb 2016 09:56:20 +0900
>> "Jaeyoon Jung" <jaeyoon.jung at lge.com> wrote:
>>
>> > > -----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.
>>
>> Right, this is a tautology: "the server wants to process more
>> events in the middle of a request handler" is the very definition of
>> recursive dispatch, not a justification for it.
>>
>>
>> > > > 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?
>>
>> I don't think it is. It would be worth to make an official statement
>> about it in the docs.
>>
>> To me it is common sense (or expert knowledge) about event loops in
>> general.
>>
>> > > > 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?
>>
>> Libwayland could prevent recursive dispatch because it has not been
>> intended to work. This patch proves that is actually was broken.
>>
>> Preventing recursive dispatch does not prevent writing a server that
>> blocks waiting on a client message. You can very well do that without
>> recursive dispatch, but it's just a little harder to write.
>>
>> > > > 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.
>>
>> You're right, we should make a statement about this. I'd be happy to
>> accept a doc patch to be included in 1.10 as AFAIK we have never
>> intended recursive dispatch to work reliably in either server or client
>> code. I do not consider it as a change in API/ABI or behaviour, just a
>> doc improvement.
>>
>> >
>> > 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.
>>
>> Raising an error like abort() is a more touchy subject, because it
>> indeed has potential to break existing programs that work by luck. We
>> probably want an escape hatch for a while, like an environment variable
>> to not abort() and just complain. Or maybe complaining is enough at
>> first and add the abort() after a couple stable releases.
>>
>> I won't propose anyone to do that work now, because I am hoping someone
>> might do it as a part of GSoC (yes, there are plans for GSoC, just not
>> announced yet I think).
>>
>> > 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 document that recursive dispatch is not supported, then we won't
>> bother with patches to fix problems with recursive dispatch, which
>> implies that we could also just revert this patch since we won't care.
>> But the patch is in, and I don't think we win anything by reverting it.
>>
>> If some usage pattern is known broken, I think it would be nice to
>> catch them and at least complain in libwayland. So far we haven't added
>> such code for even every obvious mistake, like destroying objects while
>> other objects are still referencing them.
>>
>> > > 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. ;)
>>
>> Yeah, recursive dispatch is less clear, so we should have a transition
>> period.
>>
>> > > 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 would declare recursive dispatch as illegal. Does anyone object?
>
> I don't object declaring it illegal in libwayland-server since it was
> previously broken, but doing so in libwayland-client I don't think we
> should (at least yet) since it is used here and there in various clients
> already.

Recursive dispatch sounds like an invitation to a memory leak (by
unbounded recursion).

Additionally, I'm not even sure how it'd work on the client side,
since wl_display_dispatch_queue_pending first acquires the display
lock: so doesn't a recursive dispatch deadlock?

>
>
> Jonas
>
>>
>> > >
>> > > > 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
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel