Re: deadlock in scm_join_thread(_timed)

["Julian Graham" <joolean@gmail.com>]

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

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?


> No need for a patch against both 1.8 and 1.9; just one will do, and git
> cherry-pick will handle the other for us (unless the fix is significantly
> different in the two branches).

Okay, find it attached.


Regards,
Julian

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-threads.c-join_thread_timed-Avoid-deadlock-by-a.patch --]
[-- Type: text/x-diff; name=0001-threads.c-join_thread_timed-Avoid-deadlock-by-a.patch, Size: 1467 bytes --]

From 1a90835bd4c646fb1776d6a83489820772fb5bbd Mon Sep 17 00:00:00 2001
From: Julian Graham <julian@smokebottle.(none)>
Date: Mon, 26 May 2008 21:27:24 -0400
Subject: [PATCH] * threads.c (join_thread_timed): Avoid deadlock by always checking
 thread exit status.

---
 libguile/threads.c |   32 ++++++++++++--------------------
 1 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/libguile/threads.c b/libguile/threads.c
index bf4ab16..e875cdf 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -1094,29 +1094,21 @@ SCM_DEFINE (scm_join_thread_timed, "join-thread", 1, 2, 0,
       timeout_ptr = &ctimeout;
     }
 
-  if (t->exited)
-    res = t->result;
-  else
+  while (1)
     {
-      while (1)
+      int err = 0;
+      if (t->exited)
 	{
-	  int err = block_self (t->join_queue, thread, &t->admin_mutex,
-				timeout_ptr);
-	  if (err == 0)
-	    {
-	      if (t->exited)
-		{
-		  res = t->result;
-		  break;
-		}
-	    }
-	  else if (err == ETIMEDOUT)
-	    break;
-
-	  scm_i_pthread_mutex_unlock (&t->admin_mutex);
-	  SCM_TICK;
-	  scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
+	  res = t->result;
+	  break;
 	}
+      err = block_self (t->join_queue, thread, &t->admin_mutex, timeout_ptr);
+      if (err == ETIMEDOUT)
+	break;
+
+      scm_i_pthread_mutex_unlock (&t->admin_mutex);
+      SCM_TICK;
+      scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
     }
 
   scm_i_pthread_mutex_unlock (&t->admin_mutex);
-- 
1.5.4.3