Skip Menu |
Report information
Id: 132413
Status: open
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: mauke- <l.mai [at] web.de>
Cc: leonerd [at] leonerd.org.uk
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: core
Perl Version: 5.26.0
Fixed In: (no value)



From: l.mai [...] web.de
To: perlbug [...] perl.org
Subject: PL_keyword_plugin needs thread-safe wrap setter
Date: Tue, 07 Nov 2017 18:44:36 +0100
This is a bug report for perl from l.mai@web.de, generated with the help of perlbug 1.40 running under perl 5.26.0. ----------------------------------------------------------------- [Please describe your issue here] PL_keyword_plugin (see perldoc perlapi) is global to the entire process and shared across threads. Its documentation says: | [...] take a copy of PL_keyword_plugin before assigning your own function | pointer to it. Your handler function should look for keywords that it is | interested in and handle those. Where it is not interested, it should call | the saved plugin function, passing on the arguments it received. But it does not show any example code of how this "taking a copy" is supposed to be done. The XS::APItest module in the core test suite does it the following way (excerpt): | static int (*next_keyword_plugin)(pTHX_ char *, STRLEN, OP **); | | static int my_keyword_plugin(pTHX_ | char *keyword_ptr, STRLEN keyword_len, OP **op_ptr) | { | if (...) { | ... | } else { | return next_keyword_plugin(aTHX_ keyword_ptr, keyword_len, op_ptr); | } | } | | BOOT: | { | next_keyword_plugin = PL_keyword_plugin; | PL_keyword_plugin = my_keyword_plugin; | } This code has found its way into various CPAN modules (e.g. Function::Parameters, Syntax::Keyword::Try, etc). It is also not thread-safe: % perl -Mthreads -e 'threads->create(sub { require Function::Parameters })->join for 1 .. 2' Segmentation fault This is because each thread runs BOOT separately, but PL_keyword_plugin is shared. This means the second thread BOOTing will set next_keyword_plugin to the value the first thread left in PL_keyword_plugin, which is my_keyword_plugin. Then the next call to my_keyword_plugin that enters the 'else' block will attempt to run the next handler in the chain (via 'return next_keyword_plugin(...)'), but now next_keyword_plugin == my_keyword_plugin. In other words, the list of linked handlers contains a loop, leading to a stack overflow. PL_check (the other global variable documented in perlapi) has this paragraph in its documentation: | For thread safety, modules should not write directly to this array. Instead, | use the function wrap_op_checker. This equally applies to PL_keyword_plugin, but a corresponding wrap_keyword_plugin function is missing. It should be added. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- Site configuration information for perl 5.26.0: Configured by mauke at Fri Sep 22 13:28:36 CEST 2017. Summary of my perl5 (revision 5 version 26 subversion 0) configuration: Platform: osname=linux osvers=4.9.41-1-lts archname=i686-linux uname='linux simplicio 4.9.41-1-lts #1 smp mon aug 7 17:57:02 cest 2017 i686 gnulinux ' config_args='' hint=recommended useposix=true d_sigaction=define useithreads=undef usemultiplicity=undef use64bitint=undef use64bitall=undef uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define bincompat5005=undef Compiler: cc='cc' ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64' optimize='-O2 -march=native' cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='' gccversion='7.2.0' gccosandvers='' intsize=4 longsize=4 ptrsize=4 doublesize=8 byteorder=1234 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=12 longdblkind=3 ivtype='long' ivsize=4 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=4 prototype=define Linker and Libraries: ld='cc' ldflags ='-fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/i686-pc-linux-gnu/7.2.0/include-fixed /usr/lib /lib libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.26.so so=so useshrplib=false libperl=libperl.a gnulibc_version='2.26' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags='-Wl,-E' cccdlflags='-fPIC' lddlflags='-shared -O2 -march=native -L/usr/local/lib -fstack-protector-strong' --- @INC for perl 5.26.0: /home/mauke/usr/lib/perl5/site_perl/5.26.0/i686-linux /home/mauke/usr/lib/perl5/site_perl/5.26.0 /home/mauke/usr/lib/perl5/5.26.0/i686-linux /home/mauke/usr/lib/perl5/5.26.0 --- Environment for perl 5.26.0: HOME=/home/mauke LANG=en_US.UTF-8 LANGUAGE=en_US LC_COLLATE=C LC_MONETARY=de_DE.UTF-8 LC_TIME=de_DE.UTF-8 LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/mauke/perl5/perlbrew/bin:/home/mauke/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl PERLBREW_BASHRC_VERSION=0.73 PERLBREW_HOME=/home/mauke/.perlbrew PERLBREW_ROOT=/home/mauke/perl5/perlbrew PERL_BADLANG (unset) PERL_UNICODE=SAL SHELL=/bin/bash
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Date: Wed, 8 Nov 2017 14:53:09 +0000
From: "Paul \"LeoNerd\" Evans" <leonerd [...] leonerd.org.uk>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
On Tue, 07 Nov 2017 09:44:49 -0800 l.mai@web.de (via RT) <perlbug-followup@perl.org> wrote: Show quoted text
> This code has found its way into various CPAN modules (e.g. > Function::Parameters, Syntax::Keyword::Try, etc).
In fact this issue came to light precisely because someone found it in S:K:T: https://rt.cpan.org/Ticket/Display.html?id=123547 I've applied the same fix that mauke did; namely to borrow the OP_CHECK_MUTEX for also protecting the keyword plugin hook. Since that only turned up in perl 5.14, I've had to document it as a known bug, thus: https://metacpan.org/pod/release/PEVANS/Syntax-Keyword-Try-0.09/lib/Syntax/Keyword/Try.pm#KNOWN-BUGS I'd suggest an easy fix would be to either document that this really is the suggested mechanism other modules use, or add a new mutex for them to use instead. While this fix now protects S:K:T against concurrent loading of itself, or with Function::Parameters, it won't protect against any other modules that aren't so locked. Having a common standard that all keyword plugin-using modules use is essential here. -- Paul "LeoNerd" Evans leonerd@leonerd.org.uk | https://metacpan.org/author/PEVANS http://www.leonerd.org.uk/ | https://www.tindie.com/stores/leonerd/
Download (untitled)
application/pgp-signature 195b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 609b
On Tue, 07 Nov 2017 09:44:49 -0800, mauke- wrote: Show quoted text
> > PL_check (the other global variable documented in perlapi) has this > paragraph > in its documentation: > > | For thread safety, modules should not write directly to this array. > Instead, > | use the function wrap_op_checker. > > This equally applies to PL_keyword_plugin, but a corresponding > wrap_keyword_plugin function is missing. It should be added.
I have done this in a branch: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/mauke/keyword-plugin-mutex If no one screams, I'll merge this in a couple of days (if I remember).
To: perl5-porters [...] perl.org
From: "Paul \"LeoNerd\" Evans" <leonerd [...] leonerd.org.uk>
CC: perlbug-followup [...] perl.org
Date: Thu, 9 Nov 2017 03:26:09 +0000
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Download (untitled) / with headers
text/plain 706b
On Wed, 08 Nov 2017 16:34:25 -0800 "l.mai@web.de via RT" <perlbug-followup@perl.org> wrote: Show quoted text
> I have done this in a branch: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/mauke/keyword-plugin-mutex > > If no one screams, I'll merge this in a couple of days (if I > remember).
From the perspective of a potential user of this change, I think that looks nice. It neatly wraps up the requirements nicely. One question: will there be an accompanying update to ppport.h or a suggested #ifndef-style pattern for back-compat? -- Paul "LeoNerd" Evans leonerd@leonerd.org.uk | https://metacpan.org/author/PEVANS http://www.leonerd.org.uk/ | https://www.tindie.com/stores/leonerd/
Download (untitled)
application/pgp-signature 195b

Message body not shown because it is not plain text.

Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Date: Thu, 9 Nov 2017 10:27:02 +0100
From: Sawyer X <xsawyerx [...] gmail.com>
To: "Paul \"LeoNerd\" Evans" <leonerd [...] leonerd.org.uk>, perl5-porters [...] perl.org
CC: perlbug-followup [...] perl.org
Download (untitled) / with headers
text/plain 976b
On 11/09/2017 04:26 AM, Paul "LeoNerd" Evans wrote: Show quoted text
> On Wed, 08 Nov 2017 16:34:25 -0800 > "l.mai@web.de via RT" <perlbug-followup@perl.org> wrote: >
>> I have done this in a branch: >> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/mauke/keyword-plugin-mutex >> >> If no one screams, I'll merge this in a couple of days (if I >> remember).
> From the perspective of a potential user of this change, I think that > looks nice. It neatly wraps up the requirements nicely. > > One question: will there be an accompanying update to ppport.h or a > suggested #ifndef-style pattern for back-compat?
At the moment there is no active maintainer to Devel::PPPort, so authors need to add their own backporting abilities. If it receives updates, I'm sure we could make additional releases for it. We've also discussed removing support for any Perl under 5.6. At the moment, Devel::PPPort supports 5.3. Lukas, would you be able to backport this to Devel::PPPort?
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Date: Thu, 9 Nov 2017 14:26:27 +0100
From: Lukas Mai <plokinom [...] gmail.com>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.6k
Am 09.11.2017 um 10:27 schrieb Sawyer X: Show quoted text
> On 11/09/2017 04:26 AM, Paul "LeoNerd" Evans wrote:
>> On Wed, 08 Nov 2017 16:34:25 -0800 >> "l.mai@web.de via RT" <perlbug-followup@perl.org> wrote: >>
>>> I have done this in a branch: >>> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/mauke/keyword-plugin-mutex >>> >>> If no one screams, I'll merge this in a couple of days (if I >>> remember).
>> From the perspective of a potential user of this change, I think that >> looks nice. It neatly wraps up the requirements nicely. >> >> One question: will there be an accompanying update to ppport.h or a >> suggested #ifndef-style pattern for back-compat?
> > > At the moment there is no active maintainer to Devel::PPPort, so authors > need to add their own backporting abilities. If it receives updates, I'm > sure we could make additional releases for it. > > We've also discussed removing support for any Perl under 5.6. At the > moment, Devel::PPPort supports 5.3. > > Lukas, would you be able to backport this to Devel::PPPort?
I have no idea how Devel::PPPort works. My suggestion for older perls would be: #ifndef wrap_keyword_plugin #define wrap_keyword_plugin(a,b) S_wrap_keyword_plugin(aTHX_ a,b) static void S_wrap_keyword_plugin(pTHX_ Perl_keyword_plugin_t new_plugin, Perl_keyword_plugin_t *old_plugin_p) { dVAR; PERL_UNUSED_CONTEXT; if (*old_plugin_p) return; OP_CHECK_MUTEX_LOCK; if (!*old_plugin_p) { *old_plugin_p = PL_keyword_plugin; PL_keyword_plugin = new_plugin; } OP_CHECK_MUTEX_UNLOCK; } #endif Rationale: - To be useful, the compat code must use one single shared mutex. It wouldn't help if e.g. every XS module created its own mutex. - OP_CHECK_MUTEX is technically the wrong mutex (it protects access to PL_check). But it exists, is easy to access, and the only downside is that it is slightly overprotective: Multi-threaded updates to PL_keyword_plugin and PL_check now wait for each other instead of running in parallel. I think this is harmless because the critical sections only consist of two variable assignments; there's nothing that could block for a long time. (And the situation should be very rare in the first place.) - OP_CHECK_MUTEX_LOCK / OP_CHECK_MUTEX_UNLOCK are not part of the public API. They're not documented anywhere AFAICS. However, on the perls where they exist (5.16 .. 5.26), they are stable and reliable. We can sanction their use retroactively if module authors agree not to use them where wrap_keyword_plugin is available. That way we're free to change or remove them in the future. - This leaves 5.12 and 5.14 out in the cold. I don't know what to do there. Thoughts? -- Lukas Mai <plokinom@gmail.com>
To: perl5-porters [...] perl.org
From: Zefram <zefram [...] fysh.org>
Date: Fri, 10 Nov 2017 00:05:12 +0000
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Download (untitled) / with headers
text/plain 2.5k
l.mai@web.de wrote: Show quoted text
>It is also not thread-safe:
There are really two distinct issues around thread safety, worth separating out. Firstly, there's the issue you demonstrated, that the way in which the former plugin value is saved needs to match the scope of the hook variable. This separates into two aspects in which it needs to match: storage and initialisation. Since PL_keyword_plugin is once per process, a module's saved next-plugin value also needs to be a one-per-process variable, and it needs to be initialised once per process. Storage is properly arranged by a "static" C variable. For initialisation to be once per process, unconditional initialisation in a BOOT section is, as you show, not sufficient. Some kind of conditional is required. Secondly, there's the more obscure possibility of a race condition, if two hooking operations run simultaneously in different threads. To resolve this requires the hooking critical section to be governed by a mutex. In new Perls it could be a mutex dedicated to this purpose. In existing Perls it would have to be an existing mutex that we extend to this purpose; it doesn't matter much which one we use, but it is important that we all agree on which one. The comparison to wrap_op_checker() is apt. The scoping is the same, and the way the hook variables are used is the same, so the solution will be essentially the same. Also, if you look into the history of how wrap_op_checker() came to be, it arose out of a similar conversation about thread safety. The same issue of agreeing on a mutex to use on older Perls applies too. Whereas wrap_op_checker() has its own mutex on Perls where it's in the core, there is a de facto agreement that reserve implementations of it for older Perls will use the OP_REFCNT mutex. I agree that a wrap_keyword_plugin() function would be sensible. Note that PL_keyword_plugin is flagged as experimental, so wrap_keyword_plugin() should also be so marked, being part of the same experiment. (I have things to say about where that experiment should go, but that's for another time.) With regard to which mutex to use on older Perls, I suggest using the OP_REFCNT mutex again. OP_CHECK_MUTEX would be another option where it exists, but that's only back to 5.15.8, so would require reserve definitions to be more complex, with conditional code to switch to a different mutex on pre-5.15.8 perls. OP_REFCNT is available all the way back. There's no real performance concern affecting the choice between these mutexes, just the maintainability issue. -zefram
Date: Fri, 10 Nov 2017 00:24:43 +0000
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
To: perl5-porters [...] perl.org
From: Zefram <zefram [...] fysh.org>
Download (untitled) / with headers
text/plain 351b
l.mai@web.de via RT wrote: Show quoted text
>I have done this in a branch: https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke-me/mauke/keyword-plugin-mutex > >If no one screams, I'll merge this in a couple of days (if I remember).
wrap_keyword_plugin() needs to be marked experimental, to match PL_keyword_plugin. Otherwise the patch looks good. -zefram
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 495b
On Thu, 09 Nov 2017 16:24:59 -0800, zefram@fysh.org wrote: Show quoted text
> l.mai@web.de via RT wrote:
> > I have done this in a branch: > > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- > > me/mauke/keyword-plugin-mutex > > > > If no one screams, I'll merge this in a couple of days (if I > > remember).
> > wrap_keyword_plugin() needs to be marked experimental, to match > PL_keyword_plugin. Otherwise the patch looks good.
Thanks for the review. I have pushed a new version of my branch.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Thu, 09 Nov 2017 16:05:54 -0800, zefram@fysh.org wrote: Show quoted text
> I agree that a wrap_keyword_plugin() function would be sensible. > Note that PL_keyword_plugin is flagged as experimental, so > wrap_keyword_plugin() should also be so marked, being part of the same > experiment. (I have things to say about where that experiment should go, > but that's for another time.) With regard to which mutex to use on older > Perls, I suggest using the OP_REFCNT mutex again. OP_CHECK_MUTEX would > be another option where it exists, but that's only back to 5.15.8, so > would require reserve definitions to be more complex, with conditional > code to switch to a different mutex on pre-5.15.8 perls. OP_REFCNT is > available all the way back. There's no real performance concern affecting > the choice between these mutexes, just the maintainability issue.
In light of this, my back-compat suggestion becomes: #ifndef wrap_keyword_plugin #define wrap_keyword_plugin(a, b) S_wrap_keyword_plugin(aTHX_ a, b) static void S_wrap_keyword_plugin(pTHX_ Perl_keyword_plugin_t new_plugin, Perl_keyword_plugin_t *old_plugin_p) { dVAR; PERL_UNUSED_CONTEXT; if (*old_plugin_p) return; MUTEX_LOCK(&PL_op_mutex); if (!*old_plugin_p) { *old_plugin_p = PL_keyword_plugin; PL_keyword_plugin = new_plugin; } MUTEX_UNLOCK(&PL_op_mutex); } #endif
To: perl5-porters [...] perl.org
From: Zefram <zefram [...] fysh.org>
Date: Fri, 10 Nov 2017 02:54:21 +0000
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Download (untitled) / with headers
text/plain 188b
l.mai@web.de via RT wrote: Show quoted text
>Thanks for the review. I have pushed a new version of my branch.
The entry in embed.fnc should be flagged "M" to match the "x" on the apidoc segment. -zefram
CC: perl5-porters [...] perl.org
From: Sawyer X <xsawyerx [...] gmail.com>
To: perlbug-followup [...] perl.org
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Date: Sun, 12 Nov 2017 13:38:44 +0100
Download (untitled) / with headers
text/plain 1.6k
On 11/10/2017 03:11 AM, l.mai@web.de via RT wrote: Show quoted text
> On Thu, 09 Nov 2017 16:05:54 -0800, zefram@fysh.org wrote:
>> I agree that a wrap_keyword_plugin() function would be sensible. >> Note that PL_keyword_plugin is flagged as experimental, so >> wrap_keyword_plugin() should also be so marked, being part of the same >> experiment. (I have things to say about where that experiment should go, >> but that's for another time.) With regard to which mutex to use on older >> Perls, I suggest using the OP_REFCNT mutex again. OP_CHECK_MUTEX would >> be another option where it exists, but that's only back to 5.15.8, so >> would require reserve definitions to be more complex, with conditional >> code to switch to a different mutex on pre-5.15.8 perls. OP_REFCNT is >> available all the way back. There's no real performance concern affecting >> the choice between these mutexes, just the maintainability issue.
> > In light of this, my back-compat suggestion becomes: > > #ifndef wrap_keyword_plugin > #define wrap_keyword_plugin(a, b) S_wrap_keyword_plugin(aTHX_ a, b) > static void S_wrap_keyword_plugin(pTHX_ Perl_keyword_plugin_t new_plugin, Perl_keyword_plugin_t *old_plugin_p) { > dVAR; > PERL_UNUSED_CONTEXT; > if (*old_plugin_p) return; > MUTEX_LOCK(&PL_op_mutex); > if (!*old_plugin_p) { > *old_plugin_p = PL_keyword_plugin; > PL_keyword_plugin = new_plugin; > } > MUTEX_UNLOCK(&PL_op_mutex); > } > #endif
Thank you for this patch, Lukas! (And well, ya know, the original work too :) Zefram, while you've reviewed the original patch, can you also review this back-compat patch? I'll look into merging it in Devel::PPPort. Thanks, guys!
From: "Paul \"LeoNerd\" Evans" <leonerd [...] leonerd.org.uk>
To: perl5-porters [...] perl.org
CC: perlbug-followup [...] perl.org
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Date: Sun, 12 Nov 2017 13:14:38 +0000
Download (untitled) / with headers
text/plain 1.7k
On Wed, 08 Nov 2017 16:34:25 -0800 "l.mai@web.de via RT" <perlbug-followup@perl.org> wrote: Show quoted text
> If no one screams, I'll merge this in a couple of days (if I > remember).
A thought occurs to me on this. I have two keyword-plugin-using modules on CPAN; they are Syntax::Keyword::Try Future::AsyncAwait Of those two, the first one can be made safe using this function, but the second I believe still doesn't, because it also uses a custom op. The code currently looks like BOOT: XopENTRY_set(&xop_leaveasync, xop_name, "leaveasync"); XopENTRY_set(&xop_leaveasync, xop_desc, "leaveasync()"); XopENTRY_set(&xop_leaveasync, xop_class, OA_UNOP); Perl_custom_op_register(aTHX_ &pp_leaveasync, &xop_leaveasync); XopENTRY_set(&xop_await, xop_name, "await"); XopENTRY_set(&xop_await, xop_desc, "await()"); XopENTRY_set(&xop_await, xop_class, OA_UNOP); Perl_custom_op_register(aTHX_ &pp_await, &xop_await); next_keyword_plugin = PL_keyword_plugin; PL_keyword_plugin = &my_keyword_plugin; I would need a suitable locking mechanism to lock the entire thing. Your suggested wrapper function for setting the PL_keyword_plugin hook would only protect these final two lines; I shall need something further for locking the XOP-setting code. I begin to wonder if this is a wider problem with BOOT itself; that what is needed is a way to ensure it is executed exactly once when when late-loaded in a multi-threaded environment. Maybe that can be constructed out of a suitable macro thus: static boot_once(pTHX) { ... } BOOT: ONCE(&boot_once); -- Paul "LeoNerd" Evans leonerd@leonerd.org.uk | https://metacpan.org/author/PEVANS http://www.leonerd.org.uk/ | https://www.tindie.com/stores/leonerd/
Download (untitled)
application/pgp-signature 195b

Message body not shown because it is not plain text.

Date: Mon, 13 Nov 2017 00:16:27 +0000
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
To: perl5-porters [...] perl.org
From: Zefram <zefram [...] fysh.org>
Download (untitled) / with headers
text/plain 312b
Sawyer X wrote: Show quoted text
>Zefram, while you've reviewed the original patch, can you also review >this back-compat patch?
The proposed reserve definition of wrap_keyword_plugin() is mostly correct, but should use the OP_REFCNT_{,UN}LOCK macros rather than MUTEX_{,UN}LOCK(), to be portable to unthreaded builds. -zefram
Date: Mon, 13 Nov 2017 00:20:07 +0000
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
To: perl5-porters [...] perl.org
From: Zefram <zefram [...] fysh.org>
Download (untitled) / with headers
text/plain 582b
Paul "LeoNerd" Evans wrote: Show quoted text
>I begin to wonder if this is a wider problem with BOOT itself; that >what is needed is a way to ensure it is executed exactly once when when >late-loaded in a multi-threaded environment.
No, there are parts of BOOT that need to run per interpreter. The main hidden part of an XS boot, setting up XSUB CVs, needs to be done for each interpreter in which the module is loaded, and any CV creation that's done in an explicit BOOT section needs to be treated that way too. The things that are shared between interpreters are more the exceptions. -zefram
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 810b
On Sun, 12 Nov 2017 16:20:47 -0800, zefram@fysh.org wrote: Show quoted text
> Paul "LeoNerd" Evans wrote:
> >I begin to wonder if this is a wider problem with BOOT itself; that > >what is needed is a way to ensure it is executed exactly once when when > >late-loaded in a multi-threaded environment.
> > No, there are parts of BOOT that need to run per interpreter. The main > hidden part of an XS boot, setting up XSUB CVs, needs to be done for each > interpreter in which the module is loaded, and any CV creation that's > done in an explicit BOOT section needs to be treated that way too. > The things that are shared between interpreters are more the exceptions. > > -zefram
Do you think a BOOTONCE section would be an appropriate extension to XS, or is it rare enough that we do not need it? -- Father Chrysostomos
To: perl5-porters [...] perl.org
From: Lukas Mai <plokinom [...] gmail.com>
Date: Mon, 13 Nov 2017 03:08:29 +0100
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Download (untitled) / with headers
text/plain 449b
Am 13.11.2017 um 01:16 schrieb Zefram: Show quoted text
> Sawyer X wrote:
>> Zefram, while you've reviewed the original patch, can you also review >> this back-compat patch?
> > The proposed reserve definition of wrap_keyword_plugin() is mostly > correct, but should use the OP_REFCNT_{,UN}LOCK macros rather than > MUTEX_{,UN}LOCK(), to be portable to unthreaded builds.
MUTEX_LOCK/MUTEX_UNLOCK are NOOPs on unthreaded builds. -- Lukas Mai <plokinom@gmail.com>
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Date: Mon, 13 Nov 2017 04:17:47 +0000
From: Zefram <zefram [...] fysh.org>
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 489b
Father Chrysostomos via RT wrote: Show quoted text
>Do you think a BOOTONCE section would be an appropriate extension to XS, >or is it rare enough that we do not need it?
It sounds risky, because of the greatly expanded scope of the critical section, given the need to mutex it. I think we should continue to add purpose-specific idempotence and mutexing where it's needed. For a module to require such treatment for a data structure of its own is rare enough to be not worth a new XS section. -zefram
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 839b
On Wed, 08 Nov 2017 16:34:24 -0800, mauke- wrote: Show quoted text
> On Tue, 07 Nov 2017 09:44:49 -0800, mauke- wrote:
> > > > PL_check (the other global variable documented in perlapi) has this > > paragraph > > in its documentation: > > > > | For thread safety, modules should not write directly to this array. > > Instead, > > | use the function wrap_op_checker. > > > > This equally applies to PL_keyword_plugin, but a corresponding > > wrap_keyword_plugin function is missing. It should be added.
> > I have done this in a branch: > https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- > me/mauke/keyword-plugin-mutex > > If no one screams, I'll merge this in a couple of days (if I > remember).
The branch has been merged in commit 6cc7638e57c54706dc2d698d9b2f9f769c17ffb4. Should the status of this ticket change to "pending release"?
CC: perl5-porters [...] perl.org
From: Sawyer X <xsawyerx [...] gmail.com>
To: perlbug-followup [...] perl.org
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Date: Tue, 14 Nov 2017 10:18:30 +0100
Download (untitled) / with headers
text/plain 1.1k
On 11/13/2017 08:25 PM, l.mai@web.de via RT wrote: Show quoted text
> On Wed, 08 Nov 2017 16:34:24 -0800, mauke- wrote:
>> On Tue, 07 Nov 2017 09:44:49 -0800, mauke- wrote:
>>> PL_check (the other global variable documented in perlapi) has this >>> paragraph >>> in its documentation: >>> >>> | For thread safety, modules should not write directly to this array. >>> Instead, >>> | use the function wrap_op_checker. >>> >>> This equally applies to PL_keyword_plugin, but a corresponding >>> wrap_keyword_plugin function is missing. It should be added.
>> I have done this in a branch: >> https://perl5.git.perl.org/perl.git/shortlog/refs/heads/smoke- >> me/mauke/keyword-plugin-mutex >> >> If no one screams, I'll merge this in a couple of days (if I >> remember).
> The branch has been merged in commit 6cc7638e57c54706dc2d698d9b2f9f769c17ffb4. Should the status of this ticket change to "pending release"?
Yes, but I would like to merge the backport solution to Devel::PPPort. Zefram, can you respond on Lukas' latest comment on MUTEX_LOCK/MUTEX_UNLOCK being a NOOP on unthreaded builds? Does it mean we can copy the latest version Lukas had?
To: perl5-porters [...] perl.org
From: Zefram <zefram [...] fysh.org>
Date: Tue, 14 Nov 2017 10:18:30 +0000
Subject: Re: [perl #132413] PL_keyword_plugin needs thread-safe wrap setter
Download (untitled) / with headers
text/plain 173b
Sawyer X wrote: Show quoted text
>Zefram, can you respond on Lukas' latest comment on >MUTEX_LOCK/MUTEX_UNLOCK being a NOOP on unthreaded builds?
He's right. His version is fine. -zefram


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org