[Xcb] [PATCH v2 libxcb] Prevent reply waiters from being blocked.
rami.ylimaki at vincit.fi
Wed Oct 13 07:37:03 PDT 2010
On 10/12/2010 08:46 PM, jamey at minilop.net wrote:
> I don't understand Josh's objections to this patch.
> 1) XCB_SEQUENCE_COMPARE_32 handles sequence-number wrap; we do
> inequality tests with it elsewhere, and they'd better work too.
Yes, I think that sequence number overflow was taken into account.
However, I also thought that in theory some client could execute 4
billion requests before asking a reply, but this would be a problem even
without the patch.
> 2) Even multi-reply requests are obviously not going to get any
> further responses once we've seen a higher sequence number. The
> earlier waiters will check against c->in.request_completed, and will
> not block once awakened. So waking up those threads is correct.
Would it be possible to execute xcb_record_enable_context_reply
continuously and also execute some other requests at the same time over
the same connection? This means that you would get replies with
xcb_record_enable_context sequence number even after you have already
executed some other requests using the same connection.
However, I don't think that anyone is doing this in practice. People are
usually waiting that recording has ended and only then start executing
new requests. Also the XRecord spec recommends that a separate dedicated
data connection should be used for Enable Context replies so that people
following that recommendation wouldn't face problems.
> In hindsight, your analysis and patch both look obviously correct to
> me. This might explain Havoc's xcb_request_check bug (#29599) in
> addition to maybe helping with the Xlib threading bugs.
> As far as I can tell, your patch can be simplified to just
> unconditionally call pthread_cond_signal in the loop. I'm happy to
> accept a patch doing just that if you can confirm it fixes this bug.
I'll soon send a reply to this message that contains the modified patch
and a test case that I used for testing it. The Xcb test case is based
on another Xlib test case that originally exposed the problem. However,
I won't publish the Xlib test case because it requires the
xcb_peek_for_event changes to expose the problem (the Xlib test case
doesn't advance far enough if the blocking problem in Xlib hasn't been
> I think it's possible to avoid quite a bit of work in the
> response-reading path by being a little more clever here, and I
> started trying to code up the patches I had in mind, but we should
> just merge your fix before trying to be clever.
Cool, nice to hear about this.
More information about the Xcb