[Xcb] [PATCH v2 libxcb] Prevent reply waiters from being blocked.

[Rami Ylimäki]

  Hi,

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 
fixed first).

> 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.
>
> Jamey

Cool, nice to hear about this.

-- Rami