Re: New release ? ([perl #40965])

Front page | perl.perl5.porters | Postings from January 2007

Re: New release ? ([perl #40965])

Thread Previous | Thread Next

From:

Dave Mitchell

Date:

January 15, 2007 07:14

Subject:

Re: New release ? ([perl #40965])

Message ID:

20070115151430.GD11254@iabyn.com

On Fri, Jan 05, 2007 at 10:43:04AM -0800, Jerry D. Hedden wrote:
> Rafael Garcia-Suarez wrote:
> > I'm trying to make a list of what needs to be sorted out before a new
> > development release: (5.9.5, which should be feature-equal to 5.10.0)
> >
> > - thread unsafeties ?

[snip]

> [perl #40965] Perl_op_clear is not thread-safe

which relates to this code crashing:

    ./perl -Ilib -Mthreads -e 'threads->create(sub{})->detach()'

This code now appears safe, after changes #29711, #29765, #29779, #29796,
#29805, #29810 and, #29827

The last of these changes has embedding and non-UNIX issues, which is why
I've CC'ed Jan and Stas. See below.

    Change 29711 by davem@davem-pigeon on 2007/01/08 00:06:44

        allocate op_pv strings from shared mem pool

Certain ops, most notably tr/// in non-utf8 mode, and 'next FOO' type ops,
have malloced strings attached. This change ensures they are created and
freed using the shared mem pool.

    Change 29765 by davem@davem-pigeon on 2007/01/12 10:42:05

        make tr/// threadsafe by moving swash into pad

When tr works with utf8 ranges, it uses a swash (some sort of hash) to
store the ranges, rather than a simple bit string. This used to be
attached directly to the op, in the op_sv field. This change moves it into
the pad, and duplicates it for each new thread. There was some talk of
continuing to share it (by being *really* careful), but for the 5.10
release I thought this was easier and safer.

    Change 29779 by davem@davem-pigeon on 2007/01/12 19:56:40

        Rationalise refcounting of thread structures
        Formerly there could be races with multiple destroys of a thread
        structure.

This was the much-discussed and much-evolved change to ensure no race
conditions when freeing a thread.

    Change 29796 by davem@davem-pigeon on 2007/01/13 22:57:26

        unfreed threads should trigger cleanup veto

        The thread pool struct is allocated in the main interpreter, so
        don't clean that up if any threads remain, regardless of what
        state they are in

    Change 29805 by davem@davem-pigeon on 2007/01/14 12:43:39

        further refinement to #29796 (cleanup veto)

There is a mechanism whereby the threads module can veto the cleanup of
the main interpreter: perl_destruct() calls PL_threadhook, and if that
returns true, it skips destroying the interpreter. The threads module used
to veto it if was the main thread, and if any non-detached threads still
remained. This was problematic in that: the thread pool structure is
allocated in the main thread; if there is any residual activity whatsoever
from any thread that, for example needs to access
MY_POOL.create_destruct_mutex, then it will crash and burn.

This is fixed by adding a new total_threads count, which only only gets
decremented at the very last minute of a thread's destruction. Cleanup is
vetoed if total_threads > 0. In particular, detached threads now count
against the veto, as do threads that have been joined but not yet freed.

    Change 29810 by davem@davem-pigeon on 2007/01/14 23:58:49

        make S_ithread_run() call S_ithread_free() in main context

        Fixes a race condition between detach clearing a thread's
        interpreter, and S_ithread_run() freeing itself while
        assuming that it's own interpreter still exists.

Usually S_ithread_free() is called from a different thread than the one
being freed (eg from ->detach or via mg_free); however at the end of
S_ithread_run(), we call it from ourself, and at this point, that thread's
interpreter may already have been released. (Chiefly because there is a
mutex unlock then lock between setting the PERL_ITHR_FINISHED flag and
calling S_ithread_free(), which allows detach or join to get in and free
the interpreter in the meantime.) To get round this, I have made
S_ithread_run() call S_ithread_free() with an aTHX of the main thread
rather than itself.  This trick was already done at the end of
S_ithread_free(), just before PerlMemShared_free()ing the thread
structure; I have just hitched this fix up one level.

Change 29827 by davem@davem-pigeon on 2007/01/15 14:16:53

        extend threads 'veto cleanup' to perl_free and system stuff

This is the one I would like feedback on. (the patch is included below)

Although the veto mechanism stops the main interpreter from each pool
being freed, it doesn't stop global cleanup. In particular, in the UNIX
case, main() calls PERL_SYS_TERM, which destroys all the global mutexes
like PL_op_mutex. If a detached thread happens to try to access this mutex
at around the same time that the main thread is exiting, Bad things happen.

I've fixed this by introducing a new (true) global variable,
PL_veto_cleanup, which is set in perl_destruct the first time
PL_threadhook returns true, and once set, is never unset. This flag is then
used by perl_free(), and the UNIX versions of PERL_SYS_TERM and perl_fini()
to skip any global cleanup.

An issue with this is that it's only fixed for the UNIX case; I haven't
dared mess with the win32 version (or any other version for that matter).
Also, I don't know whether there's a better approach.

Also, the original veto mechanism is per interpreter pool; I have now
extended it to be global in scope. This embedding business isn't my area
of expertise, so I'd like other eyes to look at it.

To expand on this:

A program (eg mod_perl in Apache) is capable of creating several
interpreters. Each interpreter can individually do a 'use threads'; in
which case that interpreter will become a 'main interpreter' for the pool
of threads created from that original interpreter. All threads from each
pool would share global mutexes like PL_op_mutex, but each pool would
have its own pool-wide mutexes like create_destruct_mutex, which are
allocated by that pool's main thread.

The old veto model meant that if any threads in one pool outlived
their main thread, then that one main interpreter wasn't freed. However,
after all main threads had exited, and the program itself was exiting,
then PERL_SYS_TERM and perl_fini() would still get called to free the global
mutexes and pthread keys respectively, regardless of whether there were any
detached threads still running.

The new change vetos  this global cleanup. I don't know whether that has
any bad ramifications.

Dave

==== //depot/perl/perl.c#780 (text) ====

@@ -580,6 +580,7 @@

     if (CALL_FPTR(PL_threadhook)(aTHX)) {
         /* Threads hook has vetoed further cleanup */
+       PL_veto_cleanup = TRUE;
         return STATUS_EXIT;
     }

@@ -1325,6 +1326,9 @@
 void
 perl_free(pTHXx)
 {
+    if (PL_veto_cleanup)
+       return;
+
 #ifdef PERL_TRACK_MEMPOOL
     {
        /*
@@ -1381,7 +1385,7 @@
 perl_fini(void)
 {
     dVAR;
-    if (PL_curinterp)
+    if (PL_curinterp  && !PL_veto_cleanup)
        FREE_THREAD_KEY;
 }

==== //depot/perl/perlvars.h#72 (text) ====

@@ -146,3 +146,8 @@
 #if defined(USE_ITHREADS)
 PERLVAR(Gperlio_mutex, perl_mutex)    /* Mutex for perlio fd refcounts */
 #endif
+
+/* this is currently set without MUTEX protection, so keep it a type which
+ * can be set atomically (ie not a bit field) */
+PERLVARI(Gveto_cleanup,        int, FALSE)     /* exit without cleanup */
+

==== //depot/perl/unixish.h#44 (text) ====

@@ -132,7 +132,11 @@
 #endif

 #ifndef PERL_SYS_TERM
-#  define PERL_SYS_TERM()              HINTS_REFCNT_TERM; OP_REFCNT_TERM; PERLIO_TERM; MALLOC_TERM
+#  define PERL_SYS_TERM() \
+    if (!PL_veto_cleanup) { \
+       HINTS_REFCNT_TERM; OP_REFCNT_TERM; PERLIO_TERM; MALLOC_TERM; \
+    }
+
 #endif

 #define BIT_BUCKET "/dev/null"

-- 
That he said that that that that is is is debatable, is debatable.

Thread Previous | Thread Next