Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.98k stars 560 forks source link

PL_keyword_plugin needs thread-safe wrap setter #16229

Open p5pRT opened 7 years ago

p5pRT commented 7 years ago

Migrated from rt.perl.org#132413 (status was 'open')

Searchable as RT132413$

p5pRT commented 7 years ago

From @mauke

Created by @mauke

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.

Perl Info ``` 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 ```
p5pRT commented 7 years ago

From @leonerd

On Tue\, 07 Nov 2017 09​:44​:49 -0800 l.mai@​web.de (via RT) \perlbug\-followup@​perl\.org wrote​:

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/

p5pRT commented 7 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 7 years ago

From @mauke

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).

p5pRT commented 7 years ago

From @leonerd

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?

-- Paul "LeoNerd" Evans

leonerd@​leonerd.org.uk | https://metacpan.org/author/PEVANS http​://www.leonerd.org.uk/ | https://www.tindie.com/stores/leonerd/

p5pRT commented 7 years ago

From @xsawyerx

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?

p5pRT commented 7 years ago

From @mauke

Am 09.11.2017 um 10​:27 schrieb Sawyer X​:

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

p5pRT commented 7 years ago

From zefram@fysh.org

l.mai@​web.de wrote​:

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

p5pRT commented 7 years ago

From zefram@fysh.org

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.

-zefram

p5pRT commented 7 years ago

From @mauke

On Thu\, 09 Nov 2017 16​:24​:59 -0800\, zefram@​fysh.org wrote​:

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.

p5pRT commented 7 years ago

From @mauke

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

p5pRT commented 7 years ago

From zefram@fysh.org

l.mai@​web.de via RT wrote​:

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

p5pRT commented 7 years ago

From @xsawyerx

On 11/10/2017 03​:11 AM\, l.mai@​web.de via RT wrote​:

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!

p5pRT commented 7 years ago

From @leonerd

On Wed\, 08 Nov 2017 16​:34​:25 -0800 "l.mai@​web.de via RT" \perlbug\-followup@​perl\.org wrote​:

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/

p5pRT commented 6 years ago

From zefram@fysh.org

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.

-zefram

p5pRT commented 6 years ago

From zefram@fysh.org

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

p5pRT commented 6 years ago

From @cpansprout

On Sun\, 12 Nov 2017 16​:20​:47 -0800\, zefram@​fysh.org wrote​:

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

p5pRT commented 6 years ago

From @mauke

Am 13.11.2017 um 01​:16 schrieb Zefram​:

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

p5pRT commented 6 years ago

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

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

p5pRT commented 6 years ago

From @mauke

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"?

p5pRT commented 6 years ago

From @xsawyerx

On 11/13/2017 08​:25 PM\, l.mai@​web.de via RT wrote​:

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?

p5pRT commented 6 years ago

From zefram@fysh.org

Sawyer X wrote​:

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