[PATCH wayland] client: ensure thread safety for wl_display_roundtrip_queue()

Wed Mar 2 12:25:30 UTC 2016

On Tue, 1 Mar 2016 11:41:21 -0800
Bill Spitzak <spitzak at gmail.com> wrote:

> On Thu, Feb 18, 2016 at 4:12 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 
> > On Tue, 16 Feb 2016 14:26:53 -0800
> > Bill Spitzak <spitzak at gmail.com> wrote:
> >  
> > > On Mon, Feb 15, 2016 at 7:48 PM, Jonas Ådahl <jadahl at gmail.com> wrote:
> > >
> > > The proxy wrapper approach can be tested out by appling this patch:  
> > > >  
> > https://lists.freedesktop.org/archives/wayland-devel/2015-June/023054.html  
> > >
> > >
> > > Paraphrased here:
> > >  
> > > > In short, instead of
> > > >
> > > >         bar = wl_foo_get_bar(foo);
> > > >         wl_proxy_set_queue((struct wl_proxy *) bar, queue);
> > > >         wl_bar_add_listener(bar, ...);
> > > >
> > > > with this RFC a client does
> > > >
> > > >         foo_wrapper = wl_proxy_create_wrapper((struct wl_proxy *) foo);
> > > >         wl_proxy_set_queue((struct wl_proxy *) foo_wrapper, queue);
> > > >         bar = wl_foo_get(foo_wrapper);
> > > >         wl_bar_add_listener(bar, ...);  
> > >
> > > This seems unnecessarily complex. Instead the wl_proxy can be in an
> > > unassigned state (identified by the id being set to a special value that
> > > will never be used otherwise). This avoids the extra object, allows the
> > > same code to be used even if there are different constructors for bar,  
> > and  
> > > allows other setup before the id is assigned (for instance
> > > wl_bar_add_listener is not thread safe and above examples rely on the  
> > queue  
> > > being the one belonging to the current thread):
> > >
> > >    bar = wl_bar_new();
> > >    wl_proxy_set_queue((struct wl_proxy*)bar, queue);
> > >    wl_bar_add_listener(bar, ...);
> > >    wl_foo_get_bar_id(foo, bar, ...);  
> >
> > That seems like it would work, if we were to create a completely new
> > API for handling object creation. It just changes the whole C bindings
> > API paradigm.
> >
> > Currently the C API is generated for this:
> >
> > struct wl_foo *foo_obj = ...;
> > struct wl_bar *bar_obj = wl_foo_create_bar(foo_obj);
> >
> > To implement your proposal, we have to abandon wl_foo_create_bar() as
> > legacy, and do this instead:
> >
> > struct wl_foo *foo_obj = ...;
> > struct wl_bar *bar_obj;
> >
> > bar_obj = wl_bar_new();
> > wl_proxy_set_queue((wl_proxy *)bar_obj, queue);
> > wl_bar_add_listener(bar_obj, ...);
> > /* whatever else to init bar_obj */
> > wl_foo_create_barEX(foo_obj, bar_obj);
> >
> > Yeah, that should work. And it should avoid the problem fixed in
> >
> > https://cgit.freedesktop.org/wayland/wayland/commit/?id=853c24e6998f747150e4233cf41bfa8268964cc2
> > because wl_bar_new() would not allocate an id like wl_proxy_create()
> > did.
> >
> > The problem is it's fundamentally different to the existing C bindings
> > API and even the libwayland-client ABI.
> >
> > It might be worth investigating, though. If it is possible to migrate
> > to the new API without breaking anything, it just could be worth it.
> >  
> 
> It sure looks like it would be easy to emulate the current api atop this
> new one, and thus migration should be possible without breaking anything.
> The main problem is that functions like wl_proxy_set_queue have to work
> both before and after the id is selected, however they are already doing
> the "hard" version where the id is set and messages may be coming in, so
> most of the fixes would be to avoid unnecessary locking when they are
> called without an id.
> 
> Looking at the generated documentation and how hard it is to figure out how
> to make an object, I think an even better idea is to move the "constructor"
> to the namespace most programmers expect. In the above instance it is a
> "bar" function, not a "foo" function. This changes the name and argument
> list of the last function in your example:
> 
> bar_obj = wl_bar_new();
> wl_proxy_set_queue((wl_proxy *)bar_obj, queue);
> wl_bar_add_listener(bar_obj, ...);
> /* whatever else to init bar_obj */
> wl_bar_create(bar_obj, foo_obj, ...);
> 
> Hopefully the wl_bar_create function will appear in the documentation where
> the programmer expects, in the same page as wl_bar_destroy and all the
> other wl_bar_xyz methods.

I think you have forgot the fact, that objects in the protocol sense
cannot be created out of thin air. They are always created by a request
on an object of a *different* type. Therefore the name must be
wl_foo_create_bar() for wl_foo.create_bar request, not wl_bar_create()
which would mean a wl_bar.create request. Also the "_bar" in
"create_bar" is already encoded in the XML spec, so you cannot just
mutilate it as you want.

This is not a manually implemented C function you can just arbitrarily
decide how to lay out, it must be generated from the XML description.

Thanks,
pq

> This is also nice in that no EX is needed in the name.
> 
> There are a couple instances where there is more than one constructor, and
> perhaps there already is a request called "create", in which case the best
> I can think of is to call them "create1", "create2", etc. C++ could remove
> this and rely on argument type overloading.
> 
> I'm not sure if anything can be done about objects received in events.
> Probably best to leave it as-is (the client is required to set the listener
> in the event handler, and cannot do any other changes). A link in the
> documentation from the object to the events that produce it would be nice
> however.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160302/23f7f0e6/attachment.sig>