Re: For review: pthread_cleanup_push.3

[Loic Domaigne <tech-Z4JMKDdsf89Wk0Htik3J/w@public.gmane.org>]

Gidday Michael,

let's see...

>>> A cancellation clean-up handler is popped from the stack
>>> and executed in the following circumstances:
>>> .IP 1. 3
>>> When a thread is canceled,
>>> all of the stacked clean-up handlers are popped and executed in
>>> the reverse of the order in which they were pushed onto the stack.
>>> .IP 2.
>>> When a thread terminates by calling
>>> .BR pthread_exit (3),
>>> all clean-up handlers are executed as described in the preceding point.
>>> (Clean-up handlers are \fInot\fP called if the thread terminates by
>>> performing a
>>> .I return
>>> from the thread start function.)
>> Generally speaking, it is left to the implementation to call or not
>> clean-up handlers when a thread returns from the start function.
> 
> Is it?  As far as I can see POSIX.1 just says the result is
> "undefined".  Does that mean it's up to the implementation?  I'm not
> sure.  Anyway< I added this text to NOTES:
> 
>        POSIX.1  says that the effect of using return, break, continue,
>        or   goto   to   prematurely   leave    a    block    bracketed
>        pthread_cleanup_push()  and pthread_cleanup_pop() is undefined.
>        Portable applications should avoid doing this.
> 
> Thanks for spotting this.

Tru64 implements the clean-up behavior when leaving the scope of 
pthread_cleanup_{push,pop} with a return.

Interesting c.p.t. threads on this issue:
http://groups.google.de/group/comp.programming.threads/browse_thread/thread/2a1cf7d5d44989a9/1b7431bc0e265cae
http://groups.google.de/group/comp.programming.threads/browse_thread/thread/295beb4eb09b610e/5257747c038d5162

By the way, even recognized UNIX Author errs:
http://www.apuebook.com/errata.html (Entry #13)

>>> .nf
>>> #include <pthread.h>
>>> #include <sys/types.h>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <unistd.h>
>>> #include <errno.h>
>>>
>>> #define handle_error_en(en, msg) \\
>>>        do { errno = en; perror(msg); exit(EXIT_FAILURE); } while (0)
>>>
>>> static int done = 0;
>>> static int cleanup_pop_arg = 0;
>>> static int cnt = 0;
>>>
>>> static void
>>> cleanup_handler(void *arg)
>>> {
>>>    printf("Called clean\-up handler\\n");
>>>    cnt = 0;
>>> }
>>>
>>> static void *
>>> thread_start(void *arg)
>>> {
>>>    time_t start, curr;
>>>
>>>    printf("New thread started\\n");
>>>
>>>    pthread_cleanup_push(cleanup_handler, NULL);
>>>
>>>    curr = start = time(NULL);
>>>
>>>    while (!done) {
>>>        pthread_testcancel();           /* A cancellation point */
>>>        if (curr < time(NULL)) {
>>>            curr = time(NULL);
>>>            printf("cnt = %d\\n", cnt);  /* A cancellation point */
>> printf() is not a mandatory CP (but is a CP on Linux).
> 
> Yes, but I think no change is required -- or do you think something is required.

No. Just wanted to make you aware that your program might break on 
platform where printf() is not a CP. But I Agree: this is just 
irrelevant here ;-)

>>>            cnt++;
>>>        }
>>>    }
>>>
>>>    pthread_cleanup_pop(cleanup_pop_arg);
>>>    return NULL;
>>> }
>>>
>>> int
>>> main(int argc, char *argv[])
>>> {
>>>    pthread_t thr;
>>>    int s;
>>>    void *res;
>>>
>>>    s = pthread_create(&thr, NULL, thread_start, (void *) 1);
>> Why (void*) 1 as arg?
> 
> Because I failed to clean-up after extracting code from another example...
 >
> Fixed now.

I'd have bet...

>>>    if (s != 0)
>>>        handle_error_en(s, "pthread_create");
>>>
>>>    sleep(2);           /* Allow new thread to run a while */
>>>
>>>    if (argc > 1) {
>>>        if (argc > 2)
>>>            cleanup_pop_arg = atoi(argv[2]);
>>>        done = 1;
>>>
>>>    } else {
>>>        printf("Canceling thread\\n");
>>>        s = pthread_cancel(thr);
>>>        if (s != 0)
>>>            handle_error_en(s, "pthread_cancel");
>>>    }
>>>
>>>    s = pthread_join(thr, &res);
>>>    if (s != 0)
>>>        handle_error_en(s, "pthread_join");
>>>
>>>    if (res == PTHREAD_CANCELED)
>>>        printf("Thread was canceled; cnt = %d\\n", cnt);
>>>    else
>>>        printf("Thread terminated normally; cnt = %d\\n", cnt);
>>>    exit(EXIT_SUCCESS);
>>> }
>> I see a major deficiency in your code. Unless I am mistaken, the global
>> variable <done> and <cleanup_pop_arg> are accessed from two different
>> threads.
> 
> True.
> 
>> Following the POSIX memory model, you need mutex to synchronize the
>> visibility.
> 
> Please educate me about the POSIX memory model ;-).  Say some more
> here please.  Are you meaning that the change made in main are not
> guaranteed to visible in the other thread, unless I use a
> synchronization mechanism?  (or, perhaps, a barrier?)

Oh, that's a long story. Tomorrow perhaps (I just came home, and I guess 
Antje would like to spend some time with me...)

>>> .fi
>>> .SH SEE ALSO
>>> .BR pthread_cancel (3),
>>> .BR pthread_cleanup_push_defer_np (3),
>>> .BR pthread_setcancelstate (3),
>>> .BR pthread_testcancel (3),
>>> .BR pthreads (7)
>>>
>> A last comment: pthread_join(3) and pthread_cond_wait(3) are CP, so it would
>> be nice to describe the cancellation semantic in their respective man-page.
> 
> Well, it would be nice to explain the CP semantics of every glibc
> function, but that's a prpoblem still to be resolved, right?

Yes, you are perfectly right.

Regarding the Pthreads functions:
- for pthread_cond_wait(3), you need a cleanup handler to release the 
mutex associated to the condvar if the thread currently waiting on this 
condvar is canceled.
- for pthread_join(3), you need to describe what happens to the thread 
that was about being joined by the thread you just canceled.


>> Besides that, you need clean-up handler if you use deferred cancellation on
>> pthread_cond_wait(3). Indeed, when this function is cancelled, the mutex is
>> relocked.So, you need clean-up handler to unlock the mutex.
> 
> So, I'm unclear here: what specifically are you suggesting needs to be
> changed, and on what page?

I am suggesting to describe in the man-page of pthread_cond_wait(3) what 
does happen if a thread waiting on the condvar gets canceled.

In addition, it is perhaps a good idea to add a reference <SEE also 
pthread_cond_wait(3)> for the present man-page.

HTH,
Loïc.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html