[bug #24867] `define' should be thread-safe

[bug #24867] `define' should be thread-safe

[Ludovic Courtès]

URL:
  <http://savannah.gnu.org/bugs/?24867>

                 Summary: `define' should be thread-safe
                 Project: Guile
            Submitted by: civodul
            Submitted on: Tue 18 Nov 2008 10:18:28 AM GMT
                Category: None
                Severity: 3 - Normal
              Item Group: None
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

`define' uses a hash table behind the scenes.  The hash table itself is not
thread-safe, but `define' should be thread-safe, that is, by synchronizing
accesses to the underlying hash table.

See http://thread.gmane.org/gmane.lisp.guile.devel/7851/focus=7858 for the
original report.

Thanks,
Ludo'.




    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Ludovic Courtès]

Follow-up Comment #1, bug #24867 (project guile):

Gmane appears to be broken.  Alternate URL:
http://lists.gnu.org/archive/html/guile-devel/2008-11/threads.html#00055 .

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Linas Vepstas]

Follow-up Comment #2, bug #24867 (project guile):


The following patch protects the update of the module hash tables
to be thread-safe. This is a partial solution to the bug reported 
in https://savannah.gnu.org/bugs/?24867 This is not a full solution,
because other threads might still be reading the hash tables while
they are being updated, and thus may obtain stale/bad data.

Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>

---
 libguile/modules.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: guile-1.8.6/libguile/modules.c
===================================================================
--- guile-1.8.6.orig/libguile/modules.c	2008-12-22 18:38:41.000000000 -0600
+++ guile-1.8.6/libguile/modules.c	2008-12-22 20:22:19.000000000 -0600
@@ -555,11 +555,16 @@ scm_c_define (const char *name, SCM valu
   return scm_define (scm_from_locale_symbol (name), value);
 }
 
+scm_i_pthread_mutex_t scm_i_define_mutex;
+
 SCM
 scm_define (SCM sym, SCM value)
 {
-  SCM var =
+  SCM var;
+  scm_pthread_mutex_lock(&scm_i_define_mutex);
+  var =
     scm_sym2var (sym, scm_current_module_lookup_closure (), SCM_BOOL_T);
+  scm_i_pthread_mutex_unlock(&scm_i_define_mutex);
   SCM_VARIABLE_SET (var, value);
   return var;
 }
@@ -651,6 +656,8 @@ void
 scm_init_modules ()
 {
 #include "libguile/modules.x"
+  scm_i_pthread_mutex_init (&scm_i_define_mutex, NULL);
+
   module_make_local_var_x_var = scm_c_define ("module-make-local-var!",
 					    SCM_UNDEFINED);
   scm_tc16_eval_closure = scm_make_smob_type ("eval-closure", 0);


(file #17118)
    _______________________________________________________

Additional Item Attachment:

File name: define-race.patch              Size:1 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Linas Vepstas]

Additional Item Attachment, bug #24867 (project guile):

File name: define-race.c                  Size:2 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Linas Vepstas]

Follow-up Comment #3, bug #24867 (project guile):

Note that the attached test case shows 4 distinct behaviours:

1) runs fine, exiting normally, w/o any errors

2) Spewing messages such as:
ERROR: Unbound variable: x2-347
ERROR: Unbound variable: x2-347
ERROR: Unbound variable: x2-347
ERROR: Unbound variable: define
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525
ERROR: Unbound variable: x1-2525

  the numbers being different each time, of course, but then exiting
normally.

3) Deadlocking in garbage collection, with all four threads
   stuck in 
   #5  0xf7f01f6b in scm_gc_for_newcell (freelist=0xf7f8c8ec,
    free_cells=0x90c810c) at gc.c:484

  (this may be preceeded by the prints above)

4) Segfault:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf74c5b90 (LWP 30649)]
0xf7f426ad in scm_assert_smob_type (tag=639, val=0x0) at smob.c:63
63	  if (!SCM_SMOB_PREDICATE (tag, val))
(gdb) bt
#0  0xf7f426ad in scm_assert_smob_type (tag=639, val=0x0) at smob.c:63
#1  0xf7f0b4c5 in scm_make_dynamic_state (parent=0x0) at fluids.c:508
#2  0xf7f62bdf in guilify_self_2 (parent=0x0) at threads.c:508
#3  0xf7f64992 in scm_i_init_thread_for_guile (base=0xf74c5388, parent=0x0)
    at threads.c:611
#4  0xf7f649c5 in scm_i_with_guile_and_parent (
    func=0x8048754 <guile_mode_definer>, data=0xffaf0cbc, parent=0x0)
    at threads.c:743
#5  0xf7f64ace in scm_with_guile (func=0x8048754 <guile_mode_definer>, 
    data=0xffaf0cbc) at threads.c:732
#6  0x080488a8 in definer ()
#7  0xf7fa14fb in start_thread () from /lib/tls/i686/cmov/libpthread.so.0
#8  0xf7e45e5e in clone () from /lib/tls/i686/cmov/libc.so.6


I'm guessing that the use of immutable vlists, as proposed by Ludo, for use
in the module-obarray per mail discussion, would solve all of the above.  


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Ludovic Courtès]

Follow-up Comment #4, bug #24867 (project guile):

Thanks for the patch and test case!

A few remarks:

  1. `scm_sym2var ()' must be changed to acquire the mutex before performing
actual lookup.  Thus, `scm_define ()' itself doesn't need to acquire it.  I
think this should be enough to fix the bug.  It shouldn't slow things down too
much, thanks to memoization.

  2. The mutex can be declared as `static'.

  3. In 1.9, we should perhaps avoid `scm_i_define_mutex' and use a
per-module mutex (which isn't possible in 1.8 since it would break ABI). 
OTOH, is it really necessary to have such a fine grain?

Can you provide an updated patch with ChangeLog-style entry?

Ludo'.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Linas Vepstas]

Follow-up Comment #5, bug #24867 (project guile):

1) Easier said than done, because: 

1a) the mutex needs to be recursive, since sym2var evaluates code in
boot9.scm which can cause sym2var to run again. The core problem is that the
mechanism for specifying recursive mutexes seems to be somewhat OS-dependent
(and possibly some OS'es don't support recursive mutexes??) and so a
portability wrapper might be needed. :-(

1b) There's still a strange deadlock somehow; am debugging.

3) Fine-grained usually means speedy. *if* there was some per-module C
struct, then the mutex could be put in there. (I don't know of any, but I
don't know guile internals).  The alternative would be somehow grabbing a lock
in the boot9.scm code, but I don't see how, without making some symbol lookup
(i.e. race).


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Linas Vepstas]

Follow-up Comment #6, bug #24867 (project guile):

I've attached a patch to lock the entry and exit to scm_sym2var().  However,
it doesn't fix the problem (at all).

-- There are still deadlocks in garbage collection, (but different from the
previously reported one !?)
-- There are still crashes (with same stack trace as previously posted)
-- There are still races.



(file #17129)
    _______________________________________________________

Additional Item Attachment:

File name: symvar-race.patch              Size:1 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[Kourtney Keese]

Follow-up Comment #7, bug #24867 (project guile):

thans for sharing.its very usefull.
<a href="http://www.cheap-poloshirts.com">lacoste miami</a>

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[JackBenny]

Follow-up Comment #8, bug #24867 (project guile):

Rochville University
<http://www.study-online.net/schools/rochville-university/>


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[JackBenny]

Follow-up Comment #9, bug #24867 (project guile):

Thanks a lot for sharing such an valuable stuff.
WEVAC University <http://www.facebook.com/WEVACUniversity>


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[JackBenny]

Follow-up Comment #10, bug #24867 (project guile):

Almeda University
<http://answers.yahoo.com/question/index?qid=20110416135725AAw1nCL>


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #24867] `define' should be thread-safe

[JackBenny]

Follow-up Comment #11, bug #24867 (project guile):

Belford university <http://educationfuturist.wordpress.com/>
Accredited high school diploma <http://onlinediplomahelp.com/?p=38>


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?24867>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/