[PATCH 1/2] protocol: Allow versioned message arguments

[Pekka Paalanen]

On Mon, 13 May 2013 10:23:44 +0200
Alexander Larsson <alexl at redhat.com> wrote:

> On ons, 2013-05-08 at 15:40 -0500, Jason Ekstrand wrote:
> 
> > 
> > In short, I think this is far too complex for what it achieves.  In
> > the case of scaling factor stuff, you can just do it with a second
> > event.
> 
> I agree that what I posted have some open issues, and it was mostly
> meant as a start of a discussion of how to handle this. But I completely
> disagree with this. Adding a second event is totally a bad thing. I'll
> try to explain my reasoning in this case, but also consider that over
> time this will happen in other events too, so the complexity will
> accumulate over time.
> 
> So, wl_output. This API is meant to be used to find out the initial
> monitor geometry and later dynamic updates of it. One of the primary
> consumers of this will be GdkScreen/GdkMonitor which is the Gtk+ api
> that gives this information. We never want Gtk+ to report some partial
> state to the application, so a multi-event geometry notification means
> that Gtk+ would have to have some code like:
> 
>  // just got a geometry event 
>  if (wl_output.version <= 2)
>    update GdkMonitor based on event
>    screeen.emit("changed");
>  else
>    // Can't update the data or notify until we have all parts
>    screen.saved_geometry_event_data = event;
>    screen.waiting_for_geometry_scaling_factor = 1;

This is the way to do it. As another way, rather than expecting a
specific event and pending on that indefinitely, you could issue a
roundtrip; when it returns, you are guaranteed to have received all the
additional events related to this event. OTOH, roundtripping is not
nice to use like this.

There is a slightly racy way to achieve the same, though: wait for
idle. You wait for idle in any case before re-drawing the GUI, right?

In any case, a compositor can be assumed to send all related events in
a burst, without interleaving other events. We can even specify the
order in the protocol. We already do that for other things.

> There are multiple issues here:
> * The code becomes much more complex (imagine a third event added later)

Versioning arguments of events and requests is even more complex and
fragile, and introduces automatic papering over problems with default
values.

> * Message ordering suddenly becomes important

Message ordering *is* important in general.

> * The client depends on a complex behaviour of the server (send the 2nd
>    event if wl_output.version > 2) which some compositors may not
>    implement (due to a bug or mistake) which leads to client
>    "livelocks".

This is the standard way to implement augmented protocol. If some
compositors don't do it right, they are buggy and need to be fixed.
Automatically and silently papering over the problem would make things
only harder to debug.

You negotiate the version of an interface at bind time for a reason, by
taking min(server_version, client_version).

> Or maybe I misunderstood you? Did you mean that the new event would be a
> copy of the first event with the extra field added? So, there would be
> geometry & geometry2. That would be a lot easier on the client side, all
> you have to do is listen to both and fill in a default value for the new
> fields in the handler for the old one. There are still some minor issues
> in this apprach though:
> 
> * You need to send both messages over the wire. (Technically there is
> some version negotiation when you bind the wl_output, but I don't think 
> it seems right that incrementing the output version breaks old code that
> used the old geometry event.)

It does not break. That's why the version used by both sides is
negotiated. Servers need to honour the negotiated version number
regardless.

And, there is no need to send both events, if a new event is designed
to replace an old one. Just send the event corresponding to the
negotiated version number.

> * The two events are separate things that we force the compositor to
> handle in the right way, introducing uncertainty in behaviour. With my

Umm, what uncertainty?

> patch an old server with a new client will automatically work, and a
> compositor building against a newer server library will fail to build
> until its fixed to pass the new event members. Without this you may get
> compositors that build against the new server version but still don't
> emit the new signals.

How is it a problem, if a server builds or links against a newer
libwayland-server than it was written for? It is supposed to be the
server deciding the maximum interface version that it is going to
support, not libwayland-server. Again, taking the min() of the two.

For events, it works fine. For requests, I'm not too sure whether a
buggy client could cause a server crash by sending a request from a
later interface version than it negotiated. We need a unit test for
that.

Also, with your proposal, what happens if a server is built with an old
libwayland-server, but at runtime links to a new libwayland-server?

> Now, that doesn't mean we have to implement versioned events the way my
> patch does. Its important that we get event versioning right though,
> because this is gonna show up more in the future. I think the basic
> properties we want from this are:
> 
> * It should be easy to update compositors to the new event, just switch
> to the new marshaller call with no need to manually handle back compat.
> * Old clients should keep working on the new server even though the
> server just calls the new event version marshaller.
> * New clients should be able to listen to *either* the new event version
> or the old version (i.e. allow being lazy on version update), without
> having to somehow care about which version the server is.
> 
> This can be implemented several ways though, and I agree that the way
> the patch did it is very rough on non-C languages. Another approach is
> to add a new event that "extends" the old one. I.e. like the "copy the
> event and extend it" approach above, but with support from the wayland
> machinery itself. That way the client can demarshal old events as the
> new one, and the dispatching of the new events can be done for all the
> versions of the event. This way we can avoid servers and clients having
> to manually support backwards compatibility.

As far as I know, we already have the necessary mechanims in place to
ensure libwayland API and protocol backwards compatibility, which
allows any new/old combinations server vs. client, and also program vs.
library at build vs. runtime. If you find the existing mechanisms
somehow broken in that respect, we'd like to hear about it.

I don't think versioning event/request arguments, and plugging in
arbitrary default values, is the right way.


Thanks,
pq