Hi Neil, Neil Jerram <n...@ossau.uklinux.net> writes:
> - it runs for a duration specified by environment variable > GUILE_TEST_DEFINE_RACE_DURATION > > - it defaults to just 10s if that variable isn't defined. Even 10s is going to be slightly annoying. Another option would be to create tens of guile-definer threads instead of just 4; it might trigger the problem earlier. > #2 makes the symbols hash thread-safe, and it appears that this > completely fixes the define-race problem. I don't understand why we > apparently don't need patch #3 as well - but that's what my results > indicate. Roughly #2 is a specialized version of #3 as it addresses only the case of the symbol hash table. So it should be enough for this very problem. #3 is probably the way to go in the longer run, but it changes the API/ABI so it can't go in 1.8. I'll skip #3 for now. > * test-suite/standalone/test-define-race.c: New test. Could you run `indent' on this file? > +TESTS += test-define-race It needs to be conditionalized on `--with-threads'. > scm_i_rehash (SCM table, > unsigned long (*hash_fn)(), > void *closure, > - const char* func_name) > + const char* func_name, > + scm_i_pthread_mutex_t *mutex) This function assumes that MUTEX is locked. I would either write it prominently in a comment above or change the semantics so that MUTEX is acquired in `scm_i_rehash ()', not in the caller. The latter might be achieved by integrating tests about whether TABLE needs to be rehashed into `scm_i_rehash ()' itself. I.e., this: if (SCM_HASHTABLE_N_ITEMS (table) < SCM_HASHTABLE_LOWER (table) || SCM_HASHTABLE_N_ITEMS (table) > SCM_HASHTABLE_UPPER (table)) scm_i_rehash (...); would become: scm_i_rehash (...); I haven't reviewed all uses to see whether it's actually possible and whether it would lead to race conditions, though. > -static SCM symbols; > +SCM scm_i_symbols; I don't think this is needed, is it? > +static SCM > +intern_symbol (SCM symbol, const char *name, size_t len, size_t raw_hash) Since all users of `intern_symbol ()' perform a `lookup_interned_symbol ()' right before, I'd rather change users so that they lock before `lookup' and unlock after `intern': lock (&symbols_mutex); sym = lookup_interned_symbol (name); if (scm_is_false (sym)) { sym = scm_i_c_make_symbol (...); intern_symbol (sym); } unlock (&symbols_mutex); Is it doable? Thanks, Ludo'.