develooper Front page | perl.perl5.porters | Postings from September 2013

Re: [perl #119897] glob in threads is broken in perl5.18

Thread Previous | Thread Next
From:
Brian Fraser
Date:
September 21, 2013 01:05
Subject:
Re: [perl #119897] glob in threads is broken in perl5.18
Message ID:
CA+nL+nar2A4fZQE28e2xXZtdd2_wCM1Y+HcOWh4CHOn3Vw=ahA@mail.gmail.com
On Fri, Sep 20, 2013 at 9:16 PM, Father Chrysostomos via RT <
perlbug-followup@perl.org> wrote:

> On Fri Sep 20 16:31:00 2013, Hugmeir wrote:
> > Yes. The problem for both is that x_GLOB_ENTRIES should be thread-local,
> > but is unintentionally being shared between threads.
> >
> > This patch solves the issue:
> >
> > diff --git a/ext/File-Glob/Glob.xs b/ext/File-Glob/Glob.xs
> > index b3705b3..181dbbc 100644
> > --- a/ext/File-Glob/Glob.xs
> > +++ b/ext/File-Glob/Glob.xs
> > @@ -396,6 +396,13 @@ PPCODE:
> >      iterate(aTHX_ doglob_iter_wrapper);
> >      SPAGAIN;
> >
> > +void
> > +CLONE(...)
> > +CODE:
> > +    PERL_UNUSED_ARG(items);
> > +    MY_CXT_CLONE;
> > +    MY_CXT.x_GLOB_ENTRIES = NULL;
> > +
> >  BOOT:
> >  {
> >  #ifndef PERL_EXTERNAL_GLOB
> >
> >
> > But it means that this:
> >
> > ./perl -Ilib -Mthreads -E 'sub foo {scalar glob("*")} foo(); say
> > threads->create(\&foo)->join() for 1..3'
> >
> > Will return "Artistic" four times, rather than "Artristic" followed by
> the
> > second file thrice. If the latter is the desired behavior, then we need
> to
> > hv_dup(x_GLOB_ENTRIES) in CLONE() as well.
>
> Yes, the latter.  That would be backward-compatible with when File::Glob
> still worked.
>

I'm afraid I don't know how to implement this properly! Unlike dup magic,
CLONE() doesn't get a CLONE_PARAMS argument, so calling sv_dup() and
friends requires manual intervention, and I don't believe that the correct
procedure is documented anywhere.
The only place in the core that does an sv_dup() during CLONE() is
threads.xs, and that's likely not a good role model for more normal code;
meanwhile, the paradigm in the few CPAN modules that do this[*] is "grab
the interpreter during BOOT, use that to construct a fake CLONE_PARAMS, and
then replace the interpreter with aTHX after MY_CXT_CLONE"
Which, on top of the previous patch, becomes this:

diff --git a/ext/File-Glob/Glob.xs b/ext/File-Glob/Glob.xs
index 181dbbc..118d88e 100644
--- a/ext/File-Glob/Glob.xs
+++ b/ext/File-Glob/Glob.xs
@@ -9,6 +9,9 @@
 #define MY_CXT_KEY "File::Glob::_guts" XS_VERSION

 typedef struct {
+#ifdef USE_ITHREADS
+    tTHX interp;
+#endif
     int        x_GLOB_ERROR;
     HV *    x_GLOB_ENTRIES;
     Perl_ophook_t    x_GLOB_OLD_OPHOOK;
@@ -396,12 +399,32 @@ PPCODE:
     iterate(aTHX_ doglob_iter_wrapper);
     SPAGAIN;

+#ifdef USE_ITHREADS
+
 void
 CLONE(...)
+INIT:
+    HV *glob_entries_clone = NULL;
 CODE:
     PERL_UNUSED_ARG(items);
-    MY_CXT_CLONE;
-    MY_CXT.x_GLOB_ENTRIES = NULL;
+    {
+        dMY_CXT;
+        if ( MY_CXT.x_GLOB_ENTRIES ) {
+            CLONE_PARAMS param;
+            param.stashes    = NULL;
+            param.flags      = 0;
+            param.proto_perl = MY_CXT.interp;
+
+            glob_entries_clone =
MUTABLE_HV(sv_dup_inc((SV*)MY_CXT.x_GLOB_ENTRIES, &param));
+        }
+    }
+    {
+        MY_CXT_CLONE;
+        MY_CXT.x_GLOB_ENTRIES = glob_entries_clone;
+        MY_CXT.interp = aTHX;
+    }
+
+#endif

 BOOT:
 {
@@ -418,6 +441,9 @@ BOOT:
     dMY_CXT;
     MY_CXT.x_GLOB_ENTRIES = NULL;
     MY_CXT.x_GLOB_OLD_OPHOOK = PL_opfreehook;
+#ifdef USE_ITHREADS
+        MY_CXT.interp = aTHX;
+#endif
     PL_opfreehook = glob_ophook;
     }
 }


And with that:
./perl -Ilib -Mthreads -E 'sub foo {scalar glob("*")} say foo(); say "\t",
threads->create(\&foo)->join() for 1..3; say ""; say "\t\t",
threads->create(sub { say "\t", foo(); threads->create(\&foo)->join()
})->join(); say foo()'
Artistic
    AUTHORS
    AUTHORS
    AUTHORS

    AUTHORS
        autodoc.pl
AUTHORS

So, it works, but no clue if this is the proper way of doing it.
Talking about proper ways and threads, I'm trying to write some tests for
this, but I've hit a bit of a conundrum. Test::More requires threads to be
pre-loaded to work properly with threads -- but at the same time, I want to
skip all the tests if threads aren't available. I believe the code below is
the correct way of doing that, but it means that most code out there,
including bits in the core, is doing it wrong. Does that merit patching
those files?

use Config;
use if $Config{useithreads}, 'threads';
use Test::More;

BEGIN {
    if (! $Config{'useithreads'}) {
        plan skip_all => "Perl not compiled with 'useithreads'";
    }
}



[*] At least, that's the impression that I got when I was looking some
weeks back, and what I ended up using in Params::Lazy.

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About