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

[Derek Foreman]

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

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
>