[Xcb] [PATCH 0/3] RFC: Fixes to Xlib problems.

[Rami Ylimäki]

  Hi Jamey,

Thanks for the quick review. I'll clarify the patches a little bit below.

On 10/07/2010 08:12 PM, jamey at minilop.net wrote:
> Hi, Rami!
>
> Unfortunately I have some issues with these patches.
>
> I can't accept xcb_peek_for_event because it is inherently not
> thread-safe. I didn't look too closely, but I assume that means the
> libX11 patch needs to be re-thought, right?

Could you clarify what you mean by it not being thread safe? The 
original intention of xcb_peek_for_event was to find out the sequence 
number of next event in XCB buffers. It only returns a copy of the next 
event header if an event has been read and does the copy when locked 
properly. No guarantee is given that the corresponding event exists in 
XCB after xcb_peek_for_event returns, but that should be OK.

Is the main objection against xcb_peek_for_event that it is too much 
tailored for the needs of Xlib and therefore too ugly and not good 
enough for general use? I can't see that it could be used to cause harm, 
because it only copies event header data that isn't supposed to change.

Xlib can use xcb_peek_for_event to determine whether _XReply can 
continue without handling events, provided that a call to it has been 
preceded by xcb_wait_for_reply. Calling xcb_wait_for_reply guarantees 
that XCB has seen the events preceding the reply at some point and Xlib 
can use that function to check if it should try to poll for events or if 
it can continue immediately. Did you spot some error in how Xlib is 
using xcb_peek_for_reply that would cause real threading related problems?

Sure, the Xlib patches depend on that function and have to be reworked 
if xcb_peek_for_event is not accepted. It's OK if xcb_peek_for_event is 
not accepted but I'd like to know the reason behind it: too ugly for 
public interface, is really somehow unsafe or isn't used correctly from 
Xlib.

> Regarding "Prevent event waiters from blocking reply waiters": This
> patch shouldn't be necessary. Do you have a pure-XCB test case that
> demonstrates a bug this patch fixes?
>
> Take a look at xcb_in.c:read_packet, which calls pthread_cond_signal
> on either the reader that was waiting for this reply, or on the event
> queue condition variable if it was an event. That should be sufficient
> to ensure that threads wake up when there's data for them to read.
>

In some cases xcb_in.c:read_packet doesn't signal when it could do so. I 
sent a reworked patch to the list that explains this situation.

> The Xlib threading bug here is that XCB doesn't provide any obvious
> way for Xlib to get the next event-or-reply-or-error in
> sequence-number order and block until one arrives. I've been trying to
> figure out if Xlib can use select/poll to do the blocking instead of
> letting XCB block, but the possibility that more than one thread might
> poll the same socket makes that tricky to reason about.

Does your approach involve modifying XCB interface or do you mean that 
you have been planning to modify Xlib so that a direct call to poll 
would replace current XCB wait calls and no XCB interface modifications 
would be needed? In other words, modified xcb_io.c would contain direct 
calls to poll instead of xcb_wait_for_event and xcb_wait_for_reply? Are 
you aware of some difficulties in just extending the XCB interface to 
support Xlib better or just want to avoid interface changes if possible?

> On Thu, Oct 7, 2010 at 8:43 AM, Rami Ylimäki<rami.ylimaki at vincit.fi>  wrote:
>> Jamey, we are certainly interested in making the next generation core
>> XCB available sooner than later. However, in the meanwhile ...
> Sure. :-) I'd been needing to write down those ideas anyway.
>
> Jamey

Thanks for sharing your thought for the next generation XCB. You 
definitely have some very insightful ideas.

-- Rami