Re: deadlock in scm_join_thread(_timed)

[Neil Jerram <neil@ossau.uklinux.net>]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Neil Jerram <neil@ossau.uklinux.net> writes:

> "Julian Graham" <joolean@gmail.com> writes:
>
>> Hi Neil,
>>
>>> Based on the synopsis above, I agree that moving step 1 inside the loop
>>> should fix this.  In addition, though, I think it would be very good if we
>>> could add a minimal test that currently reproduces the deadlock, and so will
>>> serve to guard against future regressions here.  Do you have such a test?
>>
>> I don't -- it seems to be pretty dependent on timing.  I noticed it
>> while running my SRFI-18 test suite in a loop, and it took hours to
>> trigger.  Any suggestions?

Here is a proposed patch for branch_release-1-8.

    Neil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-possible-deadlock-in-scm_join_thread.patch --]
[-- Type: text/x-diff, Size: 3117 bytes --]

From 66f3b6c1b043b814663668b5f83210c6e8d1e12d Mon Sep 17 00:00:00 2001
From: Neil Jerram <neil@ossau.uklinux.net>
Date: Wed, 20 May 2009 21:55:35 +0100
Subject: [PATCH] Remove possible deadlock in scm_join_thread

* libguile/threads.c (scm_join_thread): Always recheck t->exited
  before calling block_self again, in case thread t has now exited.

* test-suite/tests/threads.test (joining): New test.
---
 libguile/threads.c            |   17 +++++++----------
 test-suite/tests/threads.test |   34 +++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/libguile/threads.c b/libguile/threads.c
index fc3e607..3d6df11 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -934,17 +934,14 @@ SCM_DEFINE (scm_join_thread, "join-thread", 1, 0, 0,
   scm_i_scm_pthread_mutex_lock (&thread_admin_mutex);
 
   t = SCM_I_THREAD_DATA (thread);
-  if (!t->exited)
+  while (!t->exited)
     {
-      while (1)
-	{
-	  block_self (t->join_queue, thread, &thread_admin_mutex, NULL);
-	  if (t->exited)
-	    break;
-	  scm_i_pthread_mutex_unlock (&thread_admin_mutex);
-	  SCM_TICK;
-	  scm_i_scm_pthread_mutex_lock (&thread_admin_mutex);
-	}
+      block_self (t->join_queue, thread, &thread_admin_mutex, NULL);
+      if (t->exited)
+	break;
+      scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+      SCM_TICK;
+      scm_i_scm_pthread_mutex_lock (&thread_admin_mutex);
     }
   res = t->result;
 
diff --git a/test-suite/tests/threads.test b/test-suite/tests/threads.test
index 0146016..34ee7ee 100644
--- a/test-suite/tests/threads.test
+++ b/test-suite/tests/threads.test
@@ -133,4 +133,36 @@
 				(lambda (n) (set! result (cons n result)))
 				(lambda (n) (* 2 n))
 				'(0 1 2 3 4 5))
-	    (equal? result '(10 8 6 4 2 0)))))))
+	    (equal? result '(10 8 6 4 2 0)))))
+
+      ;;
+      ;; thread joining
+      ;;
+
+      (with-test-prefix "joining"
+
+	;; scm_join_thread has a SCM_TICK in the middle of it, to
+	;; allow asyncs to run (including signal delivery).  We used
+	;; to have a bug whereby if the joined thread terminated at
+	;; the same time as the joining thread is in this SCM_TICK,
+	;; scm_join_thread would not notice and would hang forever.
+	;; So in this test we are setting up the following sequence of
+	;; events.
+        ;;   T=0  other thread is created and starts running
+	;;   T=2  main thread sets up an async that will sleep for 10 seconds
+        ;;   T=2  main thread calls join-thread, which will...
+        ;;   T=2  ...call the async, which starts sleeping
+        ;;   T=5  other thread finishes its work and terminates
+        ;;   T=7  async completes, main thread continues inside join-thread.
+	(pass-if "don't hang when joined thread terminates in SCM_TICK"
+	  (let ((other-thread (make-thread sleep 5)))
+	    (letrec ((delay-count 10)
+		     (aproc (lambda ()
+			      (set! delay-count (- delay-count 1))
+			      (if (zero? delay-count)
+				  (sleep 5)
+				  (system-async-mark aproc)))))
+	      (sleep 2)
+	      (system-async-mark aproc)
+	      (join-thread other-thread)))
+	  #t))))
-- 
1.5.6.5