Front page | perl.perl5.porters |
Postings from September 2013
[perl #119897] glob in threads is broken in perl5.18
Thread Previous
|
Thread Next
From:
Father Chrysostomos via RT
Date:
September 21, 2013 03:19
Subject:
[perl #119897] glob in threads is broken in perl5.18
Message ID:
rt-3.6.HEAD-1873-1379733541-1792.119897-15-0@perl.org
On Fri Sep 20 18:05:44 2013, Hugmeir wrote:
> 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;
Ah, but File::Glob is *not* 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:
It has often bothered me that a CLONE method in pure Perl can’t really
access both interpreters. I have known about CLONE for a long time, but
had never actually seen it used (or bothered looking).
If what you have below works, and works reliably (it looks fine to me),
I say go for it.
Is it possible to write this in a way that is binary-compatible (for
5.16 and 5.18)? I believe the fact that the struct below is not public
makes it safe to change it like that.
>
> 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, ¶m));
> + }
> + }
> + {
> + 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?
Probably.
>
> 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.
--
Father Chrysostomos
---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119897
Thread Previous
|
Thread Next