From 05532360503cfb3668e8eda7bd6697bb817e016c Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Sat, 29 Aug 2026 15:09:32 +0200 Subject: [PATCH 1/2] Make Process.wait safe to retry after timeout --- src/pipe.toit | 45 +++++++++++++++++++++++++++++++++++++++++-- tests/pipe2_test.toit | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/src/pipe.toit b/src/pipe.toit index c77c592..71eed7e 100644 --- a/src/pipe.toit +++ b/src/pipe.toit @@ -252,9 +252,11 @@ fork use-path stdin stdout stderr command arguments -> List /** The result of forking a process with $fork. */ class Process: fork-data_/List + waiter_/ProcessWait_ exit-value_/int? := null constructor .fork-data_: + waiter_ = ProcessWait_ fork-data_[3] /** The pid of the child process. */ pid -> any: return fork-data_[3] @@ -288,7 +290,7 @@ class Process: */ wait -> int: if exit-value_ == null: - exit-value_ = wait_ pid + exit-value_ = waiter_.wait return exit-value_ static wait_ child-process -> int: @@ -302,7 +304,7 @@ class Process: Tells the system that we don't want to wait for the child process to finish. */ wait-ignore -> none: - dont-wait-for_ pid + waiter_.ignore /** Returns the exit code of the finished process. @@ -330,6 +332,45 @@ class Process: if (value & PROCESS-SIGNALLED_) == 0: return null return (value >> PROCESS-SIGNAL-SHIFT_) & PROCESS-SIGNAL-MASK_ +monitor ProcessWait_: + child-process_/any + state_/monitor.ResourceState_? := null + exit-value_/int? := null + ignored_ := false + + constructor .child-process_: + + wait -> int: + if ignored_: throw "PROCESS_WAIT_IGNORED" + if exit-value_ != null: return exit-value_ + + if state_ == null: + // Do not let a timeout land between registering the subprocess and + // retaining its ResourceState_. A retry must use the same registration. + critical-do --respect-deadline=false: + if state_ == null: + wait-for_ child-process_ + state_ = monitor.ResourceState_ process-resource-group_ child-process_ + + value := state_.wait + // Retain the result and dispose the notifier as one operation. This also + // makes concurrent and repeated waits observe the same result. + critical-do --respect-deadline=false: + if exit-value_ == null: + exit-value_ = value + state_.dispose + state_ = null + return exit-value_ + + ignore -> none: + if ignored_ or exit-value_ != null: return + critical-do --respect-deadline=false: + if state_: + state_.dispose + state_ = null + dont-wait-for_ child-process_ + ignored_ = true + /** Forks a process. diff --git a/tests/pipe2_test.toit b/tests/pipe2_test.toit index 5178946..efaa968 100644 --- a/tests/pipe2_test.toit +++ b/tests/pipe2_test.toit @@ -9,7 +9,7 @@ import host.file import host.pipe import host.pipe show windows-escape_ import semver -import system show platform PLATFORM-FREERTOS +import system show platform PLATFORM-FREERTOS PLATFORM-WINDOWS test-exit-value command args expected-exit-value sleep-time/int: complete-args := [command] + args @@ -49,6 +49,41 @@ test-exit-signal sleep-time/int: expect-equals null (pipe.exit-code exit-value) expect-equals SIGKILL (pipe.exit-signal exit-value) +test-interrupted-wait: + // Keep stdin open so the child cannot finish before the waits time out. + process := pipe.fork --create-stdin "cat" ["cat"] + + errors := List 3 + errors.size.repeat: + errors[it] = catch: with-timeout --ms=5: process.wait + + SIGKILL := 9 + pipe.kill_ process.pid SIGKILL + exit-value := with-timeout --ms=1_000: process.wait + + errors.do: expect-equals DEADLINE-EXCEEDED-ERROR it + expect-equals SIGKILL (pipe.exit-signal exit-value) + expect-equals exit-value process.wait + expect-equals SIGKILL process.exit-signal + +test-ignored-soft-kill: + // Wait for the shell to install the handler before sending the signal. + process := pipe.fork + --create-stdout + "sh" + ["sh", "-c", "trap '' TERM; echo ready; exec sleep 60"] + process.stdout.in.read + + SIGTERM := 15 + SIGKILL := 9 + pipe.kill_ process.pid SIGTERM + error := catch: with-timeout --ms=20: process.wait + pipe.kill_ process.pid SIGKILL + exit-value := with-timeout --ms=1_000: process.wait + + expect-equals DEADLINE-EXCEEDED-ERROR error + expect-equals SIGKILL (pipe.exit-signal exit-value) + main: test-windows-escaping @@ -64,6 +99,9 @@ main: test-exit-signal 0 test-exit-signal 20 + 25.repeat: test-interrupted-wait + if platform != PLATFORM-WINDOWS: test-ignored-soft-kill + // Tests a private method in pipe.toit. test-windows-escaping: expect-equals "foo" (windows-escape_ "foo") From 454170edc5f2370867476371f6b57b5a325a8bfc Mon Sep 17 00:00:00 2001 From: Florian Loitsch Date: Sat, 29 Aug 2026 15:52:45 +0200 Subject: [PATCH 2/2] Clean up abandoned process waiters --- src/pipe.toit | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/pipe.toit b/src/pipe.toit index 71eed7e..4ece557 100644 --- a/src/pipe.toit +++ b/src/pipe.toit @@ -339,15 +339,16 @@ monitor ProcessWait_: ignored_ := false constructor .child-process_: + add-finalizer this:: ignore wait -> int: if ignored_: throw "PROCESS_WAIT_IGNORED" - if exit-value_ != null: return exit-value_ + if exit-value_: return exit-value_ if state_ == null: // Do not let a timeout land between registering the subprocess and // retaining its ResourceState_. A retry must use the same registration. - critical-do --respect-deadline=false: + critical-do --no-respect-deadline: if state_ == null: wait-for_ child-process_ state_ = monitor.ResourceState_ process-resource-group_ child-process_ @@ -355,21 +356,23 @@ monitor ProcessWait_: value := state_.wait // Retain the result and dispose the notifier as one operation. This also // makes concurrent and repeated waits observe the same result. - critical-do --respect-deadline=false: - if exit-value_ == null: + critical-do --no-respect-deadline: + if not exit-value_: exit-value_ = value state_.dispose state_ = null + remove-finalizer this return exit-value_ ignore -> none: - if ignored_ or exit-value_ != null: return - critical-do --respect-deadline=false: + if ignored_ or exit-value_: return + critical-do --no-respect-deadline: if state_: state_.dispose state_ = null dont-wait-for_ child-process_ ignored_ = true + remove-finalizer this /** Forks a process.