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

Issue 1975: Fix WIN32 Proc::Async process termination issue #1995

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jstuder-gh
Copy link
Contributor

jstuder-gh commented Jun 30, 2018

Signals are emulated via libuv on Windows, so the signal handler in the
test is not called. Rather upon receiving the emulated SIGINT, libuv will just call
WIN32's TerminateProcess() call. But the process isn't terminated immediately;
the process' kernel object is not destroyed until other processes with open
handles have released them.

The process is waiting for the handles to be released before it
terminates but Proc::Async is waiting on the process terminating before
it will close it's IO handles (if any open).

Fixing this by calling done on any available suppliers within the kill
method so that the promises are no longer blocked on (in this test
the promises are being awaited). The IO handles are closed and then the
process is terminated.

Note that this solves the issue of the process hanging indefinitely, but
it does not cause the test to pass, since for it to pass the process
would need to receive the signal, execute the signal handler, and send
the right data to its stdout.

jstuder-gh added some commits Jun 30, 2018

Change 'supply' names to 'supplier' for clarity
These objects are type Supplier, so changing the name to indicate at a
glance that they are the emitters and not simply observable.
Fix WIN32 Proc::Async process termination issue
There is a test on Windows involving Proc::Async and signals that hangs
indefinitely.
See [Github Issue #1975](#1975).

Signals are emulated via libuv on Windows, so the signal handler in the
test is not called. Rather the emulated SIGINT will just call WIN32's
TerminateProcess(). But the process isn't terminated immediately; the
kernel object is not destroyed until other processes with open handles
have released them.

The process is waiting for the handles to be released before it
terminates but Proc::Async is waiting on the process terminating before
it will close it's IO handles (if any open).

Fixing this by calling done on any available suppliers within the kill
method so that the promises are no longer blocked on (in this test
the promises are being awaited). The IO handles are closed and then the
process is terminated.

Note that this solves the issue of the process hanging indefinitely, but
it does not cause the test to pass, since for it to pass the process
would need to receive the signal, execute the signal handler, and send
the right data to stdout.
@@ -412,15 +424,24 @@ my class Proc::Async {
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;
$!stdout_supplier.done() if $!stdout_supplier;

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 20, 2018

Member

Will users expect &done or &quit callbacks to be fired? I'm not really sure.

This comment has been minimized.

Copy link
@jstuder-gh

jstuder-gh Oct 25, 2018

Author Contributor

Currently, the killprocasync op invoked in the method call always cancels the work on the event queue (in MoarVM at least). However, not knowing that, it isn't unreasonable that the user might expect that the process could recover from the signal.

I was originally approaching it from the standpoint that the user is going in expecting the termination of the process, but I'm not so sure now. Might be too late to change the behavior though?

@@ -140,7 +140,19 @@ my class Proc::Async {
if the-supplier and type != value;

type = value;
the-supplier //= Supplier::Preserving.new;
the-supplier //= class :: is Supplier::Preserving {

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 20, 2018

Member

Anonymous classes were removed from the code base for performance reasons.

@@ -260,11 +272,11 @@ my class Proc::Async {

method !capture(\callbacks,\std,\the-supplier) {
my $promise = Promise.new;
my $vow = $promise.vow;
the-supplier.vow = $promise.vow;

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 20, 2018

Member

This seems race-y ( although I haven't bothered to investigate that hunch )

This comment has been minimized.

Copy link
@jstuder-gh

jstuder-gh Oct 25, 2018

Author Contributor

Looking at it again, you're right. I figure this particular method is probably alright (since it's a private method called with 3 different suppliers each time), the 'start' method could definitely cause a data race if called multiple times from different threads. I've updated the branch to add a commit wrapping it in a Lock.

I've cherry-picked that commit and will submit it on it's own, so that it can be considered regardless of what we decide to do with this PR.

EDIT:
Submitted as PR #2425

@@ -412,15 +424,24 @@ my class Proc::Async {
proto method kill(|) {*}

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 20, 2018

Member

Might be worth consolidating all the duplicated logic (including some existing) into the proto:

proto method kill(|) {
    X::Proc::Async::MustBeStarted.new(:method<kill>, proc => self).throw if !$!started;
    $!stdout_supplier.done() if $!stdout_supplier;
    $!stderr_supplier.done() if $!stderr_supplier;
    $!merge_supplier.done()  if $!merge_supplier;
    {*}
}

This comment has been minimized.

Copy link
@jstuder-gh

jstuder-gh Oct 25, 2018

Author Contributor

I had left the duplicate logic because of the comment a few lines above stating that it significantly impacted performance. That comment is quite old though so things could be different now with the improvements made to the optimizer and spesh.

jstuder-gh added some commits Oct 25, 2018

Fix potential race condition in Proc::Async start
There is potential for a race here if two threads call start on the same
Proc::Async instance. Add a lock to mitigate that risk.
Remove anonymous class in Proc::Async
Anonymous classes perform worse. ugexe++ for pointing this out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.