feat: improve process termination and directory cleanup logic - #616
feat: improve process termination and directory cleanup logic#616route wants to merge 6 commits into
Conversation
ZilvinasKucinskas
left a comment
ZilvinasKucinskas
left a comment
There was a problem hiding this comment.
I tested this head against real process groups on macOS and Debian. The blocking issue below reproduces on the current code and passes with the suggested loop. The candidate also passes Ferrum's full suite and lint; the async API note is optional.
| ::Process.kill("USR1", pid) | ||
| send_signal(pid, "TERM") | ||
| start = Utils::ElapsedTime.monotonic_time | ||
| while ::Process.wait(pid, ::Process::WNOHANG).nil? |
There was a problem hiding this comment.
This returns as soon as the process-group leader exits. If Chrome exits on TERM while a renderer or zygote stays alive, wait reaps the leader and group KILL is never sent. I reproduced this on macOS and Debian with a child in the same group that ignores TERM (child_alive=true on this commit).
Please track leader reaping separately and keep polling the group until it exits or the timeout sends KILL:
leader_exited = false
loop do
leader_exited ||= !::Process.wait(pid, ::Process::WNOHANG).nil?
break if leader_exited && !process_group_alive?(pid)
sleep(WAIT_KILLED)
next unless Utils::ElapsedTime.timeout?(start, KILL_TIMEOUT)
send_signal(pid, "KILL")
::Process.wait(pid) unless leader_exited
break
endprocess_group_alive? can use Process.kill(0, -pid), treating ESRCH as false and EPERM as true. Please add one real subprocess regression; the current mocks always return nil and miss this case.
There was a problem hiding this comment.
thats an oversight likely from my side, looking at your patch gist. ported it over too.
| # | ||
| # @return [void] | ||
| # | ||
| def self.remove_directory(path, retries: REMOVE_DIR_RETRIES, delay: REMOVE_DIR_RETRY_DELAY) |
There was a problem hiding this comment.
Optional: with the 2s browser timeout plus 1.5s directory backoff, quit can block for about 3.5s, or 5.5s with Xvfb. Could we expose an opt-in Browser#quit(wait: false) / Process#stop(wait: false)?
Please keep wait: true as the default and keep restart synchronous, so Ferrum and Cuprite behavior stays unchanged. The async path should close CDP synchronously, run browser group -> Xvfb -> user-data-directory cleanup through Utils::Thread.spawn(abort_on_exception: false), and return the thread or handle so callers can join it before process exit or fixed-port reuse. We currently carry this as an application patch and could remove it.
- Add `send_signal` method for robust PID and process group signaling. - Implement `remove_directory` with retries and backoff to handle transient errors.
- Add `process_group_alive?` method to detect the state of a process group. - Enhance process termination logic to handle scenarios where group members ignore TERM signals. - Add tests to verify process group termination behavior.
- Enhance `#quit` and `#stop` methods to support non-blocking cleanup with `wait: false`. - Introduce `sync_stop` and `async_stop` for handling synchronous and asynchronous logic.
- Move process termination, signal handling, and directory cleanup methods to `Process::Killer`. - Simplify `Process` class by delegating termination and cleanup to the new module.
9dde5f8 to
e1c4737
Compare
2 participants
send_signalmethod for robust PID and process group signaling.remove_directorywith retries and backoff to handle transient errors.