diff mbox

[4/5] nptl: Fix sem_wait and sem_timedwait cancellation

Message ID 1471876053-780-4-git-send-email-adhemerval.zanella@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Netto Aug. 22, 2016, 2:27 p.m. UTC 
  This patch fixes both sem_wait and sem_timedwait cancellation point for
uncontended case.  In this scenario only atomics are involved and thus
the futex cancellable call is not issue and a pending cancellation signal
is not handled.

The fix is straighforward by calling pthread_testcancel is both function
start.  Although it would be simpler to call CANCELLATION_P directly, I
decided to add an internal pthread_testcancel alias and use it to export
less internal implementation on such function.  A possible change on
how pthread_testcancel is internally implemented would lead to either
continue to force use CANCELLATION_P or to adjust its every use.

GLIBC testcases do have tests for uncontended cases, test-cancel12
and test-cancel14.c, however both are flawed by adding another
cancellation point just after thread pthread_cleanup_pop:

 47 static void *
 48 tf (void *arg)
 49 {
 50   pthread_cleanup_push (cleanup, NULL);
 51
 52   int e = pthread_barrier_wait (&bar);
 53   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
 54     {
 55       puts ("tf: 1st barrier_wait failed");
 56       exit (1);
 57     }
 58
 59   /* This call should block and be cancelable.  */
 60   sem_wait (&sem);
 61
 62   pthread_cleanup_pop (0);
 63
 64   puts ("sem_wait returned");
 65
 66   return NULL;
 67 }

So sem_{timed}wait does not act on cancellation, pthread_cleanup_pop executes
'cleanup' and then 'puts' acts on cancellation.  Since pthread_cleanup_pop
removed the clean-up handler, it will ran only once and thus it won't accuse
an error to indicate sem_wait has not acted on the cancellation signal.

This patch also fixes this behavior by removing the cancellation point 'puts'.
It also adds some cleanup on all sem_{timed}wait cancel tests, mostly to avoid
calling 'exit' and some error formatting.

It partially fixes BZ #18243.  Checked on x86_64.

	[BZ #18243]
	* nptl/pthreadP.h (__pthread_testcancel): Add prototype and hidden_proto.
	* nptl/pthread_testcancel.c (pthread_cancel): Add internal aliais
	definition.
	* nptl/sem_timedwait.c (sem_timedwait): Add cancellation check for
	uncontended case.
	* nptl/sem_wait.c (__new_sem_wait): Likewise.
	* nptl/tst-cancel12.c (ncall): New global variable.
	(thread_ret): Likewise.
	(clean): Remove cancellation points.
	(tf): Fix check for uncontended case and remove exit calls.
	(do_test): Likewise.
	* nptl/tst-cancel13.c (ncall): New global variable.
	(thread_ret): Likewise.
	(clean): Remove cancellation points.
	(tf): Fix check for uncontended case and remove exit calls.
	(do_test): Likewise.
	* nptl/tst-cancel14.c (ncall): New global variable.
	(thread_ret): Likewise.
	(clean): Remove cancellation points.
	(tf): Fix check for uncontended case and remove exit calls.
	(do_test): Likewise.
	* nptl/tst-cancel15.c (ncall): New global variable.
	(thread_ret): Likewise.
	(clean): Remove cancellation points.
	(tf): Fix check for uncontended case and remove exit calls.
	(do_test): Likewise.
---
 nptl/pthreadP.h           |  2 ++
 nptl/pthread_testcancel.c |  4 ++-
 nptl/sem_timedwait.c      |  3 +++
 nptl/sem_wait.c           |  3 +++
 nptl/tst-cancel12.c       | 64 +++++++++++++++++++++++------------------------
 nptl/tst-cancel13.c       | 49 ++++++++++++++++++++----------------
 nptl/tst-cancel14.c       | 43 ++++++++++++++++---------------
 nptl/tst-cancel15.c       | 53 +++++++++++++++++++++------------------
 9 files changed, 148 insertions(+), 101 deletions(-)

Comments

Torvald Riegel Sept. 5, 2016, 6:07 p.m. UTC | #1 
On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> This patch fixes both sem_wait and sem_timedwait cancellation point for
> uncontended case.  In this scenario only atomics are involved and thus
> the futex cancellable call is not issue and a pending cancellation signal
> is not handled.

I have added a comment on the BZ explaining why I think this is NOTABUG.
Essentially, if we don't need to suspend or block, there's no suspension
or blocking we need to be able to cancel.  Adding a one-time check is
not reliable anyway because the cancellation request could be incoming
right after this check.

Also, if we don't want to make assumptions about timing, there is no way
to send a cancellation request reliably after sem_wait has reached it's
cancellation point because sem_wait doesn't communicate when it started
(or reached the cancellation point).

I don't see much value to allow a user to cancel something that would
never wait, because the user already is able to never call sem_wait at
all.  Furthermore, relying on cancellation to take effect when sem_wait
would never wait would break if the user code called a sem_trywait right
before the sem_wait, which arguably would be quite inconsistent
semantics.

Another point to consider is that even if glibc's implementation added
bounded(!) spinning (without checking for cancellation) before
eventually using futexes with cancellation enabled, we would still
fulfill the cancellation requirements because we can argue that the
cancellation point just happens a little later.

> GLIBC testcases do have tests for uncontended cases, test-cancel12
> and test-cancel14.c, however both are flawed by adding another
> cancellation point just after thread pthread_cleanup_pop:

I think both tests should just be removed because they check for
behavior we do not need to implement.  Sorry for not realizing this when
I updated the semaphore implementation.
Torvald Riegel Sept. 9, 2016, 4:32 p.m. UTC | #2 
On Mon, 2016-09-05 at 20:07 +0200, Torvald Riegel wrote:
> On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> > This patch fixes both sem_wait and sem_timedwait cancellation point for
> > uncontended case.  In this scenario only atomics are involved and thus
> > the futex cancellable call is not issue and a pending cancellation signal
> > is not handled.
> 
> I have added a comment on the BZ explaining why I think this is NOTABUG.

I've looked at the POSIX rationale again, and it seems I was wrong in my
interpretation of what the POSIX spec requires.  I now think that we
need to add the pthread_testcancel call or equivalent.  The patch looks
okay, but please also add a comment to the added pthread_testcancel call
explaining why we need it.  Something like this may be useful:

We need to check whether we need to act upon a cancellation request here
because POSIX specifies that cancellation points "shall occur" in
sem_wait and sem_timedwait, which also means that they need to check
this regardless whether they block or not (unlike "may occur"
functions).  See the POSIX Rationale for this requirement: Section
"Thread Cancellation Overview" in
http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
See http://austingroupbugs.net/view.php?id=1076 for thoughts on why this
may be a suboptimal design.


Thanks.
Adhemerval Zanella Netto Sept. 14, 2016, 8:50 p.m. UTC | #3 
On 09/09/2016 13:32, Torvald Riegel wrote:
> On Mon, 2016-09-05 at 20:07 +0200, Torvald Riegel wrote:
>> On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
>>> This patch fixes both sem_wait and sem_timedwait cancellation point for
>>> uncontended case.  In this scenario only atomics are involved and thus
>>> the futex cancellable call is not issue and a pending cancellation signal
>>> is not handled.
>>
>> I have added a comment on the BZ explaining why I think this is NOTABUG.
> 
> I've looked at the POSIX rationale again, and it seems I was wrong in my
> interpretation of what the POSIX spec requires.  I now think that we
> need to add the pthread_testcancel call or equivalent.  The patch looks
> okay, but please also add a comment to the added pthread_testcancel call
> explaining why we need it.  Something like this may be useful:
> 
> We need to check whether we need to act upon a cancellation request here
> because POSIX specifies that cancellation points "shall occur" in
> sem_wait and sem_timedwait, which also means that they need to check
> this regardless whether they block or not (unlike "may occur"
> functions).  See the POSIX Rationale for this requirement: Section
> "Thread Cancellation Overview" in
> http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
> See http://austingroupbugs.net/view.php?id=1076 for thoughts on why this
> may be a suboptimal design.
> 

Thanks for check out this, my initial understanding followed your #4 comment
in bug report.  I will add your comment in the patch.

> 
> Thanks.
>
diff mbox

Patch

diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 4edc74b..6e0dd09 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -491,6 +491,7 @@  extern int __pthread_setcanceltype (int type, int *oldtype);
 extern int __pthread_enable_asynccancel (void) attribute_hidden;
 extern void __pthread_disable_asynccancel (int oldtype)
      internal_function attribute_hidden;
+extern void __pthread_testcancel (void);
 
 #if IS_IN (libpthread)
 hidden_proto (__pthread_mutex_init)
@@ -505,6 +506,7 @@  hidden_proto (__pthread_getspecific)
 hidden_proto (__pthread_setspecific)
 hidden_proto (__pthread_once)
 hidden_proto (__pthread_setcancelstate)
+hidden_proto (__pthread_testcancel)
 #endif
 
 extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond);
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 51be09f..fdef699 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -21,7 +21,9 @@ 
 
 
 void
-pthread_testcancel (void)
+__pthread_testcancel (void)
 {
   CANCELLATION_P (THREAD_SELF);
 }
+strong_alias (__pthread_testcancel, pthread_testcancel)
+hidden_def (__pthread_testcancel)
diff --git a/nptl/sem_timedwait.c b/nptl/sem_timedwait.c
index 1aab25a..8253021 100644
--- a/nptl/sem_timedwait.c
+++ b/nptl/sem_timedwait.c
@@ -30,6 +30,9 @@  sem_timedwait (sem_t *sem, const struct timespec *abstime)
       return -1;
     }
 
+  /* Check cancellation for uncontended case.  */
+  __pthread_testcancel ();
+
   if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
     return 0;
   else
diff --git a/nptl/sem_wait.c b/nptl/sem_wait.c
index 84b523a..4928c7d 100644
--- a/nptl/sem_wait.c
+++ b/nptl/sem_wait.c
@@ -23,6 +23,9 @@ 
 int
 __new_sem_wait (sem_t *sem)
 {
+  /* Check cancellation for uncontended case.  */
+  __pthread_testcancel ();
+
   if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
     return 0;
   else
diff --git a/nptl/tst-cancel12.c b/nptl/tst-cancel12.c
index ac8f5a0..9d93ddb 100644
--- a/nptl/tst-cancel12.c
+++ b/nptl/tst-cancel12.c
@@ -1,4 +1,5 @@ 
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_wait cancellation for uncontended case.
+   Copyright (C) 2003-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
 
@@ -23,104 +24,101 @@ 
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
-
+#include <stdbool.h>
 
 static pthread_barrier_t bar;
 static sem_t sem;
-
+static int ncall;
+static volatile int thread_ret;
 
 static void
 cleanup (void *arg)
 {
-  static int ncall;
-
   if (++ncall != 1)
     {
       puts ("second call to cleanup");
-      exit (1);
+      thread_ret = 1;
     }
-
-  printf ("cleanup call #%d\n", ncall);
 }
 
-
 static void *
 tf (void *arg)
 {
+  thread_ret = 0;
+  ncall = 0;
+
   pthread_cleanup_push (cleanup, NULL);
 
   int e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
-      puts ("tf: 1st barrier_wait failed");
-      exit (1);
+      puts ("error: tf: 1st barrier_wait failed");
+      thread_ret = 1;
+      return NULL;
     }
 
-  /* This call should block and be cancelable.  */
   sem_wait (&sem);
 
   pthread_cleanup_pop (0);
 
-  puts ("sem_wait returned");
-
   return NULL;
 }
 
-
 static int
 do_test (void)
 {
   pthread_t th;
 
+  thread_ret = 0;
+
   if (pthread_barrier_init (&bar, NULL, 2) != 0)
     {
-      puts ("barrier_init failed");
-      exit (1);
+      puts ("error: barrier_init failed");
+      return 1;
     }
 
+  /* A value higher than 0 will check for uncontended pthread cancellation,
+     where the sem_wait operation will return immediatelly.  */
   if (sem_init (&sem, 0, 1) != 0)
     {
-      puts ("sem_init failed");
-      exit (1);
+      puts ("error: sem_init failed");
+      return 1;
     }
 
   if (pthread_create (&th, NULL, tf, NULL) != 0)
     {
-      puts ("create failed");
-      exit (1);
+      puts ("error: create failed");
+      return 1;
     }
 
-  /* Check whether cancellation is honored even before sem_wait does
-     anything.  */
   if (pthread_cancel (th) != 0)
     {
-      puts ("1st cancel failed");
-      exit (1);
+      puts ("error: pthread_cancel failed");
+      return 1;
     }
 
   int e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
-      puts ("1st barrier_wait failed");
-      exit (1);
+      puts ("error: 1st barrier_wait failed");
+      return 1;
     }
 
   void *r;
   if (pthread_join (th, &r) != 0)
     {
-      puts ("join failed");
-      exit (1);
+      puts ("error: pthread_join failed");
+      return 1;
     }
 
   if (r != PTHREAD_CANCELED)
     {
-      puts ("thread not canceled");
-      exit (1);
+      puts ("error: thread not canceled");
+      return 1;
     }
 
-  return 0;
+  return thread_ret;
 }
 
-
 #define TEST_FUNCTION do_test ()
 #include "../test-skeleton.c"
diff --git a/nptl/tst-cancel13.c b/nptl/tst-cancel13.c
index c84780d..7c7fd6b 100644
--- a/nptl/tst-cancel13.c
+++ b/nptl/tst-cancel13.c
@@ -1,4 +1,5 @@ 
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_wait cancellation for contended case.
+   Copyright (C) 2003-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
 
@@ -27,6 +28,8 @@ 
 
 static pthread_barrier_t bar;
 static sem_t sem;
+static int ncall;
+static volatile int thread_ret;
 
 
 static void
@@ -37,23 +40,24 @@  cleanup (void *arg)
   if (++ncall != 1)
     {
       puts ("second call to cleanup");
-      exit (1);
+      thread_ret = 1;
     }
-
-  printf ("cleanup call #%d\n", ncall);
 }
 
-
 static void *
 tf (void *arg)
 {
+  thread_ret = 0;
+  ncall = 0;
+
   pthread_cleanup_push (cleanup, NULL);
 
   int e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
-      puts ("tf: 1st barrier_wait failed");
-      exit (1);
+      puts ("error: tf: 1st barrier_wait failed");
+      thread_ret = 1;
+      return NULL;
     }
 
   /* This call should block and be cancelable.  */
@@ -61,8 +65,6 @@  tf (void *arg)
 
   pthread_cleanup_pop (0);
 
-  puts ("sem_wait returned");
-
   return NULL;
 }
 
@@ -71,30 +73,33 @@  static int
 do_test (void)
 {
   pthread_t th;
+  thread_ret = 0;
 
   if (pthread_barrier_init (&bar, NULL, 2) != 0)
     {
-      puts ("barrier_init failed");
-      exit (1);
+      puts ("error: barrier_init failed");
+      return 1;
     }
 
+  /* A value equal to 0 will check for contended pthread cancellation,
+     where the sem_wait operation will block.  */
   if (sem_init (&sem, 0, 0) != 0)
     {
-      puts ("sem_init failed");
-      exit (1);
+      puts ("error: sem_init failed");
+      return 1;
     }
 
   if (pthread_create (&th, NULL, tf, NULL) != 0)
     {
-      puts ("create failed");
-      exit (1);
+      puts ("error: create failed");
+      return 1;
     }
 
   int e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
-      puts ("1st barrier_wait failed");
-      exit (1);
+      puts ("error: 1st barrier_wait failed");
+      return 1;
     }
 
   /* Give the child a chance to go to sleep in sem_wait.  */
@@ -103,15 +108,15 @@  do_test (void)
   /* Check whether cancellation is honored when waiting in sem_wait.  */
   if (pthread_cancel (th) != 0)
     {
-      puts ("1st cancel failed");
-      exit (1);
+      puts ("error: 1st cancel failed");
+      return 1;
     }
 
   void *r;
   if (pthread_join (th, &r) != 0)
     {
-      puts ("join failed");
-      exit (1);
+      puts ("error: join failed");
+      return 1;
     }
 
   if (r != PTHREAD_CANCELED)
@@ -120,7 +125,7 @@  do_test (void)
       exit (1);
     }
 
-  return 0;
+  return thread_ret;
 }
 
 
diff --git a/nptl/tst-cancel14.c b/nptl/tst-cancel14.c
index 3810d2b..dbd0394 100644
--- a/nptl/tst-cancel14.c
+++ b/nptl/tst-cancel14.c
@@ -1,4 +1,5 @@ 
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_timedwait cancellation for uncontended case.
+   Copyright (C) 2003-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
 
@@ -28,33 +29,35 @@ 
 
 static pthread_barrier_t bar;
 static sem_t sem;
+static int ncall;
+static volatile int thread_ret;
 
 
 static void
 cleanup (void *arg)
 {
-  static int ncall;
-
   if (++ncall != 1)
     {
       puts ("second call to cleanup");
-      exit (1);
+      thread_ret = 1;
     }
-
-  printf ("cleanup call #%d\n", ncall);
 }
 
 
 static void *
 tf (void *arg)
 {
+  thread_ret = 0;
+  ncall = 0;
+
   pthread_cleanup_push (cleanup, NULL);
 
   int e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
       puts ("tf: 1st barrier_wait failed");
-      exit (1);
+      thread_ret = 1;
+      return NULL;
     }
 
   struct timeval tv;
@@ -71,8 +74,6 @@  tf (void *arg)
 
   pthread_cleanup_pop (0);
 
-  puts ("sem_timedwait returned");
-
   return NULL;
 }
 
@@ -82,44 +83,46 @@  do_test (void)
 {
   pthread_t th;
 
+  thread_ret = 0;
+
   if (pthread_barrier_init (&bar, NULL, 2) != 0)
     {
-      puts ("barrier_init failed");
-      exit (1);
+      puts ("error: barrier_init failed");
+      return 1;
     }
 
   if (sem_init (&sem, 0, 1) != 0)
     {
-      puts ("sem_init failed");
-      exit (1);
+      puts ("error: sem_init failed");
+      return 1;
     }
 
   if (pthread_create (&th, NULL, tf, NULL) != 0)
     {
-      puts ("create failed");
-      exit (1);
+      puts ("error: create failed");
+      return 1;
     }
 
   /* Check whether cancellation is honored even before sem_timedwait does
      anything.  */
   if (pthread_cancel (th) != 0)
     {
-      puts ("1st cancel failed");
-      exit (1);
+      puts ("error: 1st cancel failed");
+      return 1;
     }
 
   int e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
       puts ("1st barrier_wait failed");
-      exit (1);
+      return 1;
     }
 
   void *r;
   if (pthread_join (th, &r) != 0)
     {
       puts ("join failed");
-      exit (1);
+      return 1;
     }
 
   if (r != PTHREAD_CANCELED)
@@ -128,7 +131,7 @@  do_test (void)
       exit (1);
     }
 
-  return 0;
+  return thread_ret;
 }
 
 
diff --git a/nptl/tst-cancel15.c b/nptl/tst-cancel15.c
index f413839..c86164e 100644
--- a/nptl/tst-cancel15.c
+++ b/nptl/tst-cancel15.c
@@ -1,4 +1,5 @@ 
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_timedwait cancellation for contended case.
+   Copyright (C) 2003-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
 
@@ -28,26 +29,27 @@ 
 
 static pthread_barrier_t bar;
 static sem_t sem;
+static int ncall;
+static volatile int thread_ret;
 
 
 static void
 cleanup (void *arg)
 {
-  static int ncall;
-
   if (++ncall != 1)
     {
       puts ("second call to cleanup");
-      exit (1);
+      thread_ret = 1;
     }
-
-  printf ("cleanup call #%d\n", ncall);
 }
 
 
 static void *
 tf (void *arg)
 {
+  thread_ret = 0;
+  ncall = 0;
+
   int e;
 
   pthread_cleanup_push (cleanup, NULL);
@@ -55,8 +57,9 @@  tf (void *arg)
   e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
-      puts ("tf: 1st barrier_wait failed");
-      exit (1);
+      puts ("error: tf: 1st barrier_wait failed");
+      thread_ret = 1;
+      return NULL;
     }
 
   struct timeval tv;
@@ -74,8 +77,6 @@  tf (void *arg)
 
   pthread_cleanup_pop (0);
 
-  printf ("sem_timedwait returned, e = %d, errno = %d\n", e, errno);
-
   return NULL;
 }
 
@@ -85,29 +86,31 @@  do_test (void)
 {
   pthread_t th;
 
+  thread_ret = 0;
+
   if (pthread_barrier_init (&bar, NULL, 2) != 0)
     {
-      puts ("barrier_init failed");
-      exit (1);
+      puts ("error: barrier_init failed");
+      return 1;
     }
 
   if (sem_init (&sem, 0, 0) != 0)
     {
-      puts ("sem_init failed");
-      exit (1);
+      puts ("error: sem_init failed");
+      return 1;
     }
 
   if (pthread_create (&th, NULL, tf, NULL) != 0)
     {
-      puts ("create failed");
-      exit (1);
+      puts ("error: create failed");
+      return 1;
     }
 
   int e = pthread_barrier_wait (&bar);
   if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
     {
-      puts ("1st barrier_wait failed");
-      exit (1);
+      puts ("error: 1st barrier_wait failed");
+      return 1;
     }
 
   /* Give the child a chance to go to sleep in sem_wait.  */
@@ -116,24 +119,24 @@  do_test (void)
   /* Check whether cancellation is honored when waiting in sem_timedwait.  */
   if (pthread_cancel (th) != 0)
     {
-      puts ("1st cancel failed");
-      exit (1);
+      puts ("error: 1st cancel failed");
+      return 1;
     }
 
   void *r;
   if (pthread_join (th, &r) != 0)
     {
-      puts ("join failed");
-      exit (1);
+      puts ("error: join failed");
+      return 1;
     }
 
   if (r != PTHREAD_CANCELED)
     {
-      puts ("thread not canceled");
-      exit (1);
+      puts ("error: thread not canceled");
+      return 1;
     }
 
-  return 0;
+  return thread_ret;
 }