Re: srfi-18 requirements

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

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

Hi Neil,

> > Of course.
>
> Good, thanks.

Find attached a patch against HEAD that includes only the bug fix
stuff (2 deadlocks and use of thread-specific admin mutex) from the
original patch, modified to change make_jmpbuf instead of the signal
delivery code.

With regard to your comments about the rest of the patch, agreed, except:

Given the similarities between the existing Guile threading code and
SRFI-18, what level of comptability between these two domains will be
supported?  For example, SRFI-18 specifies that threads waiting on a
mutex held by a thread that exits should be notified of the exit and
that one of them will then be able to lock that mutex.  Given the
changes you describe below, will this behavior only work if all the
components in the user's code were created using the SRFI-18 API?

What about a thread that calls SRFI-18's thread-join function on a
non-SRFI-18 thread that died with an exception?  (Are you sure you
don't want thread exceptions in the core?  I feel like join-thread
isn't really "complete" without them...)


> Finally, there are quite a few spurious changes (or perhaps just that
> I don't understand yet) in patch: whitespace, line break and docstring
> changes.  Can you revert all these, as they only confuse the overall
> picture?

This may have just been stuff that Ludovic asked me to clean up (or
that I just cleaned up ad-hoc).  It can all go.


> I'm sorry that I'm asking for some significant changes here, but I
> hope that you'll agree that they make the enhancement clearer, and in
> particular that it is a good thing to reduce the changes that we need
> to make to the C code.  Please let me know what you think.

Not a problem.  Thank you for taking the time to all this analysis.
What's the next thing you'd like me to submit?  How about (2), the
enhancement patch for timed joins?


Regards,
Julian

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: srfi-18-bugs.HEAD.patch --]
[-- Type: text/x-patch; name=srfi-18-bugs.HEAD.patch, Size: 7407 bytes --]

Index: libguile/ChangeLog
===================================================================
RCS file: /sources/guile/guile/guile-core/libguile/ChangeLog,v
retrieving revision 1.2421
diff -a -u -r1.2421 ChangeLog
--- libguile/ChangeLog	29 Dec 2007 01:35:33 -0000	1.2421
+++ libguile/ChangeLog	6 Jan 2008 21:11:29 -0000
@@ -1,3 +1,15 @@
+2008-01-06  Julian Graham  <joolean@gmail.com>
+
+	* threads.c (scm_enter_guile): Lock `thread_admin_mutex' when entering 
+	guile mode to prevent a race during GC.
+	(do_thread_exit, scm_cancel_thread, scm_set_thread_cleanup_x, 
+	scm_thread_cleanup): Lock on thread-specific admin mutex instead of 
+	`thread_admin_mutex'.
+	* threads.h (scm_i_thread)[admin_mutex]: New field.
+	* throw.c (make_jmpbuf): Don't enter critical section during thread
+	spawn -- there is a possibility of deadlock if other threads are
+	exiting.
+	
 2007-12-29  Neil Jerram  <neil@ossau.uklinux.net>
 
 	* gc.c (mark_gc_async): Change "func_data" to "fn_data", to avoid
Index: libguile/threads.c
===================================================================
RCS file: /sources/guile/guile/guile-core/libguile/threads.c,v
retrieving revision 1.90
diff -a -u -r1.90 threads.c
--- libguile/threads.c	20 Oct 2007 11:09:58 -0000	1.90
+++ libguile/threads.c	6 Jan 2008 21:11:29 -0000
@@ -1,4 +1,4 @@
-/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2002, 2003, 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
  * 
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -369,14 +369,22 @@
 
 typedef void* scm_t_guile_ticket;
 
+static scm_i_pthread_mutex_t thread_admin_mutex = SCM_I_PTHREAD_MUTEX_INITIALIZER;
+
 static void
 scm_enter_guile (scm_t_guile_ticket ticket)
 {
   scm_i_thread *t = (scm_i_thread *)ticket;
   if (t)
     {
+      /* The admin mutex must be locked here to prevent the thread from
+	 entering guile-mode after scm_thread_go_to_sleep has been set to 1 in
+	 scm_i_thread_go_to_sleep. */
+
+      scm_i_pthread_mutex_lock (&thread_admin_mutex);
       scm_i_pthread_mutex_lock (&t->heap_mutex);
       resume (t);
+      scm_i_pthread_mutex_unlock (&thread_admin_mutex);
     }
 }
 
@@ -401,7 +409,6 @@
   return (scm_t_guile_ticket) t;
 }
 
-static scm_i_pthread_mutex_t thread_admin_mutex = SCM_I_PTHREAD_MUTEX_INITIALIZER;
 static scm_i_thread *all_threads = NULL;
 static int thread_count;
 
@@ -435,6 +442,7 @@
   /* XXX - check for errors. */
   pipe (t->sleep_pipe);
   scm_i_pthread_mutex_init (&t->heap_mutex, NULL);
+  scm_i_pthread_mutex_init (&t->admin_mutex, NULL);
   t->clear_freelists_p = 0;
   t->gc_running_p = 0;
   t->canceled = 0;
@@ -494,7 +502,7 @@
 				      scm_handle_by_message_noexit, NULL);
     }
 
-  scm_i_scm_pthread_mutex_lock (&thread_admin_mutex);
+  scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
 
   t->exited = 1;
   close (t->sleep_pipe[0]);
@@ -502,7 +510,7 @@
   while (scm_is_true (unblock_from_queue (t->join_queue)))
     ;
 
-  scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+  scm_i_pthread_mutex_unlock (&t->admin_mutex);
 
   return NULL;
 }
@@ -931,15 +939,15 @@
 
   SCM_VALIDATE_THREAD (1, thread);
   t = SCM_I_THREAD_DATA (thread);
-  scm_i_scm_pthread_mutex_lock (&thread_admin_mutex);
+  scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
   if (!t->canceled)
     {
       t->canceled = 1;
-      scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+      scm_i_pthread_mutex_unlock (&t->admin_mutex);
       scm_i_pthread_cancel (t->pthread);
     }
   else
-    scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+    scm_i_pthread_mutex_unlock (&t->admin_mutex);
 
   return SCM_UNSPECIFIED;
 }
@@ -957,13 +965,13 @@
   if (!scm_is_false (proc))
     SCM_VALIDATE_THUNK (2, proc);
 
-  scm_i_pthread_mutex_lock (&thread_admin_mutex);
-
   t = SCM_I_THREAD_DATA (thread);
+  scm_i_pthread_mutex_lock (&t->admin_mutex);
+
   if (!(t->exited || t->canceled))
     t->cleanup_handler = proc;
 
-  scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+  scm_i_pthread_mutex_unlock (&t->admin_mutex);
 
   return SCM_UNSPECIFIED;
 }
@@ -979,10 +987,10 @@
 
   SCM_VALIDATE_THREAD (1, thread);
 
-  scm_i_pthread_mutex_lock (&thread_admin_mutex);
   t = SCM_I_THREAD_DATA (thread);
+  scm_i_pthread_mutex_lock (&t->admin_mutex);
   ret = (t->exited || t->canceled) ? SCM_BOOL_F : t->cleanup_handler;
-  scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+  scm_i_pthread_mutex_unlock (&t->admin_mutex);
 
   return ret;
 }
@@ -1001,24 +1009,24 @@
   if (scm_is_eq (scm_current_thread (), thread))
     SCM_MISC_ERROR ("cannot join the current thread", SCM_EOL);
 
-  scm_i_scm_pthread_mutex_lock (&thread_admin_mutex);
-
   t = SCM_I_THREAD_DATA (thread);
+  scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
+
   if (!t->exited)
     {
       while (1)
 	{
-	  block_self (t->join_queue, thread, &thread_admin_mutex, NULL);
+	  block_self (t->join_queue, thread, &t->admin_mutex, NULL);
 	  if (t->exited)
 	    break;
-	  scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+	  scm_i_pthread_mutex_unlock (&t->admin_mutex);
 	  SCM_TICK;
-	  scm_i_scm_pthread_mutex_lock (&thread_admin_mutex);
+	  scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
 	}
     }
   res = t->result;
 
-  scm_i_pthread_mutex_unlock (&thread_admin_mutex);
+  scm_i_pthread_mutex_unlock (&t->admin_mutex);
 
   return res;
 }
Index: libguile/threads.h
===================================================================
RCS file: /sources/guile/guile/guile-core/libguile/threads.h,v
retrieving revision 1.49
diff -a -u -r1.49 threads.h
--- libguile/threads.h	20 Oct 2007 11:09:58 -0000	1.49
+++ libguile/threads.h	6 Jan 2008 21:11:29 -0000
@@ -3,7 +3,7 @@
 #ifndef SCM_THREADS_H
 #define SCM_THREADS_H
 
-/* Copyright (C) 1996,1997,1998,2000,2001, 2002, 2003, 2004, 2006, 2007 Free Software Foundation, Inc.
+/* Copyright (C) 1996,1997,1998,2000,2001, 2002, 2003, 2004, 2006, 2007, 2008 Free Software Foundation, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -52,6 +52,9 @@
 
   SCM cleanup_handler;
   SCM join_queue;
+
+  scm_i_pthread_mutex_t admin_mutex;
+
   SCM result;
   int canceled;
   int exited;
Index: libguile/throw.c
===================================================================
RCS file: /sources/guile/guile/guile-core/libguile/throw.c,v
retrieving revision 1.114
diff -a -u -r1.114 throw.c
--- libguile/throw.c	22 Jan 2007 15:14:40 -0000	1.114
+++ libguile/throw.c	6 Jan 2008 21:11:29 -0000
@@ -1,4 +1,4 @@
-/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2003, 2004, 2006 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2003, 2004, 2006, 2008 Free Software Foundation, Inc.
  * 
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -75,12 +75,8 @@
 make_jmpbuf (void)
 {
   SCM answer;
-  SCM_CRITICAL_SECTION_START;
-  {
-    SCM_NEWSMOB2 (answer, tc16_jmpbuffer, 0, 0);
-    SETJBJMPBUF(answer, (jmp_buf *)0);
-    DEACTIVATEJB(answer);
-  }
-  SCM_CRITICAL_SECTION_END;
+  SCM_NEWSMOB2 (answer, tc16_jmpbuffer, 0, 0);
+  SETJBJMPBUF(answer, (jmp_buf *)0);
+  DEACTIVATEJB(answer);
   return answer;
 }