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

Clean up child process exits on Windows #3817

Merged
merged 4 commits into from
Aug 4, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .release-notes/3187.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
## Clean up child process exits on Windows

Fixed the `ProcessMonitor` class sometimes waiting on an invalid process handle on Windows.
35 changes: 23 additions & 12 deletions packages/process/_process.pony
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ primitive _WaitPidStatus
class _ProcessWindows is _Process
let hProcess: USize
let processError: (ProcessError | None)
var finalWaitResult: (_WaitResult | None) = None
Comment on lines 348 to +350
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per the style guide, we use snake case for fields.

I'll open an issue for that.

https://github.com/ponylang/ponyc/blob/main/STYLE_GUIDE.md#naming

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened: #3818


new create(
path: String,
Expand Down Expand Up @@ -429,17 +430,27 @@ class _ProcessWindows is _Process
end

fun ref wait(): _WaitResult =>
if hProcess != 0 then
var exit_code: I32 = 0
match @ponyint_win_process_wait(hProcess, addressof exit_code)
| 0 => Exited(exit_code)
| 1 => _StillRunning
| let code: I32 =>
// we might want to propagate that code to the user, but should it do
// for other errors too
WaitpidError
end
match finalWaitResult
| let wr: _WaitResult =>
wr
else
WaitpidError
var wr: _WaitResult = WaitpidError
if hProcess != 0 then
var exit_code: I32 = 0
match @ponyint_win_process_wait(hProcess, addressof exit_code)
| 0 =>
wr = Exited(exit_code)
finalWaitResult = wr
wr
| 1 => _StillRunning
| let code: I32 =>
// we might want to propagate that code to the user, but should it do
// for other errors too
finalWaitResult = wr
wr
end
else
finalWaitResult = wr
wr
end
end

49 changes: 31 additions & 18 deletions packages/process/process_monitor.pony
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ actor ProcessMonitor
var _timers: (Timers tag | None) = None
let _process_poll_interval: U64

var _pollingChild: Bool = false
var _finalWaitResult: (_WaitResult | None) = None

new create(
auth: ProcessMonitorAuth,
backpressure_auth: BackpressureAuth,
Expand Down Expand Up @@ -262,25 +265,35 @@ actor ProcessMonitor
end

be _wait_for_child() =>
match _child.wait()
| _StillRunning =>
let timers = _ensure_timers()
let pm: ProcessMonitor tag = this
let tn =
object iso is TimerNotify
fun ref apply(timer: Timer, count: U64): Bool =>
pm._wait_for_child()
true
match _finalWaitResult
| let wr: _WaitResult =>
None
else
match _child.wait()
| let sr: _StillRunning =>
if not _pollingChild then
_pollingChild = true
let timers = _ensure_timers()
let pm: ProcessMonitor tag = this
let tn =
object iso is TimerNotify
fun ref apply(timer: Timer, count: U64): Bool =>
pm._wait_for_child()
true
end
let timer = Timer(consume tn, _process_poll_interval, _process_poll_interval)
timers(consume timer)
end
let timer = Timer(consume tn, _process_poll_interval, _process_poll_interval)
timers(consume timer)
| let exit_status: ProcessExitStatus =>
// process child exit code or termination signal
_notifier.dispose(this, exit_status)
_dispose_timers()
| let wpe: WaitpidError =>
_notifier.failed(this, ProcessError(WaitpidError))
_dispose_timers()
| let exit_status: ProcessExitStatus =>
// process child exit code or termination signal
_finalWaitResult = exit_status
_notifier.dispose(this, exit_status)
_dispose_timers()
| let wpe: WaitpidError =>
_finalWaitResult = wpe
_notifier.failed(this, ProcessError(WaitpidError))
_dispose_timers()
end
end

fun ref _ensure_timers(): Timers tag =>
Expand Down
5 changes: 4 additions & 1 deletion src/libponyrt/lang/process.c
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ PONY_API int32_t ponyint_win_process_wait(size_t hProcess, int32_t* exit_code_pt
int32_t retval = 0;

// just poll
switch (WaitForSingleObject((HANDLE)hProcess, 0))
DWORD result = WaitForSingleObject((HANDLE)hProcess, 0);
switch (result)
{
case WAIT_OBJECT_0: // process exited
if (GetExitCodeProcess((HANDLE)hProcess, (DWORD*)exit_code_ptr) == 0)
Expand All @@ -144,6 +145,8 @@ PONY_API int32_t ponyint_win_process_wait(size_t hProcess, int32_t* exit_code_pt
retval = GetLastError();
if (retval == 0) retval = -1;
break;
default:
break;
}

CloseHandle((HANDLE)hProcess);
Expand Down