[Xcb] [PATCH 0/3] RFC: Fixes to Xlib problems.
rami.ylimaki at vincit.fi
Fri Oct 8 06:00:20 PDT 2010
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
> 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.
Thanks for sharing your thought for the next generation XCB. You
definitely have some very insightful ideas.
More information about the Xcb