Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PL_keyword_plugin needs thread-safe wrap setter #16229

Open
p5pRT opened this issue Nov 7, 2017 · 22 comments
Open

PL_keyword_plugin needs thread-safe wrap setter #16229

p5pRT opened this issue Nov 7, 2017 · 22 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Nov 7, 2017

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

Searchable as RT132413$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Nov 7, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 8, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 8, 2017

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Nov 9, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 9, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 9, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 9, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 10, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 10, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 10, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 10, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 10, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 12, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 12, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 13, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 13, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 13, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 13, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 13, 2017

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
Copy link
Author

@p5pRT p5pRT commented Nov 13, 2017

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 6cc7638. Should the status of this ticket change to "pending release"?

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Nov 14, 2017

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 6cc7638. 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
Copy link
Author

@p5pRT p5pRT commented Nov 14, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant