Permalink
Browse files

Fix Proc::Async.kill failing to kill sometimes

Fixes RT#131479: https://rt.perl.org/Ticket/Display.html?id=131479
Fixes roast/#158: perl6/roast#158

The observed problems are due to multiple .kills in separate threads
ending up calling $*KERNEL.signal, which isn't thread safe, ending
up in it returning a type object, crashing due to return type constraint

Fix by first refactoring[^1] $*KERNEL.signal so only the Str:D
candidate is not thread safe and then refactoring Proc::Async.kill
to use multies for non-Str signals, so we don't have to use
$*KERNEL.signal for those, as for Str signals, we use a lock when calling
$*KERNEL.signal, avoiding race issues.

[1] 79b8ab9
  • Loading branch information...
zoffixznet committed Jun 2, 2017
1 parent 01d948d commit 99421d4caa05ae952020a6d918f94fc7b68f2305
Showing with 19 additions and 3 deletions.
  1. +18 −2 src/core/Proc/Async.pm
  2. +1 −1 tools/build/moar_core_sources
@@ -221,8 +221,24 @@ my class Proc::Async {
True;
}

method kill(Proc::Async:D: $signal = "HUP") {
# Note: some of the duplicated code in methods could be moved to
# proto, but at the moment (2017-06-02) that makes the call 24% slower
proto method kill(|) { * }
multi method kill(Proc::Async:D: Signal:D \signal = SIGHUP) {
X::Proc::Async::MustBeStarted.new(:method<kill>, proc => self).throw if !$!started;
nqp::killprocasync($!process_handle, $*KERNEL.signal($signal));
nqp::killprocasync($!process_handle, signal.value)
}
multi method kill(Proc::Async:D: Int:D \signal) {
X::Proc::Async::MustBeStarted.new(:method<kill>, proc => self).throw if !$!started;
nqp::killprocasync($!process_handle, signal)
}

# $*KERNEL.signal with Str:D signal isn't thread-safe, as it initializes
# a hash attribute, so we stick this operation into a lock
my $kill-lock = Lock.new;
multi method kill(Proc::Async:D: Str:D \signal) {
X::Proc::Async::MustBeStarted.new(:method<kill>, proc => self).throw if !$!started;
nqp::killprocasync($!process_handle,
$kill-lock.protect: { $*KERNEL.signal: signal })
}
}
@@ -149,8 +149,8 @@ src/core/IO/Socket.pm
src/core/IO/Socket/INET.pm
src/core/IO/Socket/Async.pm
src/core/Proc.pm
src/core/Proc/Async.pm
src/core/signals.pm
src/core/Proc/Async.pm
src/core/Systemic.pm
src/core/VM.pm
src/core/Distro.pm

2 comments on commit 99421d4

@jnthn

This comment has been minimized.

Copy link
Member

jnthn replied Jun 2, 2017

This would have been more robust to fix in whatever is behind $*KERNEL.signal, I think. Otherwise, we'll just see it again as soon as some other threaded code tries to use that.

@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet replied Jun 2, 2017

That was my first goal, but basically entire Kernel.pm is thread-unsafe because methods lazily instantiate all the attributes.

Making them get their values on instantiation would might be too expensive since shelling out is involved. And my async-fu is currently not strong enough to come up with some in-between solution.

Please sign in to comment.