[Xcb] [RFC] [PATCH 0/4] pthread cancellation

[Rémi Denis-Courmont]

Le jeudi 7 janvier 2010 21:48:53 Jamey Sharp, vous avez écrit :
> Thanks for the patches, Rémi!
> 
> 2010/1/7 Rémi Denis-Courmont <remi at remlab.net>:
> > As sorta promised last month, here is a partial patch series for libxcb
> > to support POSIX thread cancellation. Of course, this only works with
> > deferred cancellation. (Asynchronous cancellation is safe only with pure
> > computation and no calls to the C run-time. It only makes sense in long
> > computationally- intensive code paths.)
> 
> That makes sense. If we were going to be thorough, would we need to
> add pthread_setcanceltype calls everywhere to force deferred
> cancellation mode? Or can we assume that callers will set async
> cancellation only for blocks where that's safe?

A new POSIX thread defaults to deferred mode. The application is supposed to 
switch to asynchronous cancellation whenever appropriate. Indeed, none of 
POSIX function calls are (specified to be) safe to call under asynchronous 
cancellation type. For the sake of consistency and simplicity, I'd follow the 
same approach with XCB calls - let the caller turn to deferred cancellation 
type if it ever changes the type.

> > The first patch disables cancellation around every mandatory POSIX
> > cancellation points, optional POSIX cancellation points, and external
> > library calls (Xau and Xdmcp). It is self-sufficient. With it, XCB
> > becomes safe with regards to deferred thread cancellation, by not having
> > any cancellation point.
> 
> Bleh. The pthread API for this is painful. I'm supportive of this
> patch in principle, but practically speaking it introduces a lot of
> clutter.
> 
> Is there any way we can get the same effect with less code?

Churn can be greatly reduced by squashing the first patch into the subsequent 
ones. But ultimately, you have to either disable cancellation *or* install 
cleanup handlers for any allocated resource around any cancellation point.

Also, cleanup blocks cannot be branched out of.

So yes, this requires a bit of code.

> Also, Ian Osgood points out: we need pthread_setcancelstate added to
> pthread-stubs for this patch, right? GNU libc includes both
> pthread_setcancelstate and pthread_setcanceltype, so adding them to
> pthread-stubs still won't produce any code on glibc systems. And as
> far as I can tell from reading POSIX, in a single-threaded program
> it's safe to return 0 without doing anything--except that perhaps the
> oldstate out-param should get filled in with a sensible default if
> it's non-NULL?

Using cancellation in a single-threaded program does not make much sense. 
pthread_setcancelstate and pthread_setcanceltype could be as simple as 
proxying a static variable. pthread_cleanup_(push|pop) is a tiny bit more 
involved - it is _not_ a no-op if the parameter to pthread_cleanup_pop() is 
true.

> Would you put together the pthread-stubs patch too?
>
> Finally, another point Ian raised: How can we tell whether you've made
> every call safe? Is there any good way to either test this or be
> convinced by inspection? Massive bonus points if you add tests to
> XCB's (rather pathetic) test suite.

AFAICT, resource leak is the most likely consequence of bug in that regard. 
Testing those is not easy, unless you can instrument the functions doing 
resource allocation and releasing. Namely one'd need its own malloc/free, 
getaddrinfo/freeaddrinfo, socket/close functions to do accounting :/

> > The next patches introduces proper cancellation support for parts of the
> > connect and disconnect code paths. This is easier to start with than the
> > I/O paths. Obviously, this is incomplete.
> 
> Patches 2, 3, and 4 look fine to me. They are:
> 
> Reviewed-by: Jamey Sharp <jamey at minilop.net>
> 
> Jamey
> 


-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis