Skip to content

Commit

Permalink
[rb] Handle graceful webdriver shutdown (#14430)
Browse files Browse the repository at this point in the history
When shutting down `Selenium::WebDriver::ServiceManager#stop_server`
issues a `/shutdown` request against the webdriver server and the
server exits cleanly, however the mechanism to assert if the child
process is up or not cannot distinguish between busy or not found.

`Selenium::WebDriver::ChildProcess#exited?` returns `false` when the
process is still alive because
`Selenium::WebDriver::ChildProcess#waitpid2` returns `nil`.

However, by catching `Errno::ECHILD` and `Errno::ESRCH` and returning
`nil` `#waitpid2` masks a not running pid as "running" delaying the
shutdown procedure when the server was already gone.

This patch moves the exception handling away from the `Process` wrappers
so its closer to the decision points in
`Selenium::WebDriver::ChildProcess#exited?` and
`Selenium::WebDriver::ChildProcess#stop`.

Addresses a similar inconsistency in #14032
  • Loading branch information
josegomezr authored Nov 15, 2024
1 parent b0464e1 commit 7f4d4bf
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 14 deletions.
34 changes: 20 additions & 14 deletions rb/lib/selenium/webdriver/common/child_process.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,10 @@ def stop(timeout = 3)
return unless @pid
return if exited?

WebDriver.logger.debug("Sending TERM to process: #{@pid}", id: :process)
terminate(@pid)
poll_for_exit(timeout)

WebDriver.logger.debug(" -> stopped #{@pid}", id: :process)
rescue TimeoutError, Errno::EINVAL
WebDriver.logger.debug(" -> sending KILL to process: #{@pid}", id: :process)
kill(@pid)
wait
WebDriver.logger.debug(" -> killed #{@pid}", id: :process)
terminate_and_wait_else_kill(timeout)
rescue Errno::ECHILD, Errno::ESRCH => e
# Process exited earlier than terminate/kill could catch
WebDriver.logger.debug(" -> process: #{@pid} does not exist (#{e.class.name})", id: :process)
end

def alive?
Expand All @@ -91,6 +85,9 @@ def exited?
WebDriver.logger.debug(" -> exit code is #{exit_code.inspect}", id: :process)

!!exit_code
rescue Errno::ECHILD, Errno::ESRCH
WebDriver.logger.debug(" -> process: #{@pid} already finished", id: :process)
true
end

def poll_for_exit(timeout)
Expand All @@ -110,20 +107,29 @@ def wait

private

def terminate_and_wait_else_kill(timeout)
WebDriver.logger.debug("Sending TERM to process: #{@pid}", id: :process)
terminate(@pid)
poll_for_exit(timeout)

WebDriver.logger.debug(" -> stopped #{@pid}", id: :process)
rescue TimeoutError, Errno::EINVAL
WebDriver.logger.debug(" -> sending KILL to process: #{@pid}", id: :process)
kill(@pid)
wait
WebDriver.logger.debug(" -> killed #{@pid}", id: :process)
end

def terminate(pid)
Process.kill(SIGTERM, pid)
end

def kill(pid)
Process.kill(SIGKILL, pid)
rescue Errno::ECHILD, Errno::ESRCH
# already dead
end

def waitpid2(pid, flags = 0)
Process.waitpid2(pid, flags)
rescue Errno::ECHILD
# already dead
end
end # ChildProcess
end # WebDriver
Expand Down
1 change: 1 addition & 0 deletions rb/lib/selenium/webdriver/common/service_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ def stop_process
def stop_server
connect_to_server do |http|
headers = WebDriver::Remote::Http::Common::DEFAULT_HEADERS.dup
WebDriver.logger.debug('Sending shutdown request to server', id: :driver_service)
http.get('/shutdown', headers)
end
end
Expand Down

0 comments on commit 7f4d4bf

Please sign in to comment.