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.j...@lge.com> wrote: > > > > -----Original Message----- > > > From: Derek Foreman [mailto:der...@osg.samsung.com] > > > Sent: Wednesday, February 03, 2016 6:56 AM > > > To: Pekka Paalanen; Jaeyoon Jung > > > Cc: 'Jonas Ådahl'; wayland-devel@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.j...@lge.com> wrote: > > > > > > > >>> -----Original Message----- From: wayland-devel > > > >>> [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf > > > >>> Of Jonas Adahl Sent: Tuesday, January 12, 2016 12:27 PM To: > > > >>> Jaeyoon Jung Cc: wayland-devel@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.j...@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. 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel