diff --git a/.rubocop.yml b/.rubocop.yml index d04c8713..e9b9e3f6 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -6,6 +6,10 @@ AllCops: Layout/FirstArrayElementIndentation: EnforcedStyle: consistent +Layout/LineLength: + Exclude: + - spec/**/* + Naming/PredicateMethod: Enabled: false diff --git a/CHANGELOG.md b/CHANGELOG.md index 525d1a41..2e080999 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ ## [Unreleased](https://github.com/rubycdp/ferrum/compare/v0.18.0...main) ## ### Added +- `Ferrum::Browser#quit`/`Ferrum::Browser::Process#stop` accept `wait: false` to return immediately and run process + killing and user-data-directory cleanup on a background thread instead of blocking; the call returns the `Thread` + so callers can `#join` it if they need cleanup to have finished, e.g. before process exit or before reusing a + fixed port. Default (`wait: true`) keeps the previous synchronous behavior; `#restart` always waits. ### Changed @@ -9,6 +13,16 @@ A worker's attach or service-worker detach that timed out (e.g. on a loaded CI runner) would escape unrescued into `Client::Subscriber`'s dispatch thread and kill it, silently breaking further `Target.*` event delivery for the rest of that browser's life, raising `Ferrum::NoSuchTargetError` +- `Ferrum::Browser::Process` killed the browser's leader pid alone with `SIGUSR1`, a signal Chromium has no shutdown + handler for; it now sends `SIGTERM`, escalating to `SIGKILL`, to the whole process group, so renderer/GPU/zygote + child processes are no longer orphaned after `#quit`. A leader process that exited promptly on `TERM` used to + short-circuit escalation to `KILL` for the rest of its process group, so a child that ignored `TERM` + (e.g. a stuck renderer) was left running forever; termination now keeps polling the group until it's actually empty + or the timeout fires. +- Removing the user data directory after `#quit` silently gave up on any error, potentially leaking the temp + directory forever with no indication; it now retries with exponential backoff on transient errors + (`Errno::ENOTEMPTY`/`EBUSY`/`EACCES`/`EPERM`, since Chrome can briefly hold file locks right after being killed) + and warns if it still can't be removed. ### Removed diff --git a/docs/1-introduction.md b/docs/1-introduction.md index 2940a061..e807db03 100644 --- a/docs/1-introduction.md +++ b/docs/1-introduction.md @@ -118,6 +118,18 @@ browser.reset browser.quit ``` +`#quit` blocks by default until the browser process is confirmed dead and its user data directory removed. Killing a +stubborn process (one that ignores `TERM` and needs `KILL`) can take a couple of seconds, which matters if you're +quitting many browsers in a hot path. Pass `wait: false` to return immediately and run that cleanup on a background +thread instead: + +```ruby +browser = Ferrum::Browser.new +thread = browser.quit(wait: false) +# ... do other work while the browser is killed and its directory removed in the background ... +thread.join # only needed if you must wait for cleanup to finish, e.g. before reusing a fixed port +``` + ## Thread safety Ferrum is fully thread-safe. You can create one browser or a few as you wish and diff --git a/lib/ferrum/browser.rb b/lib/ferrum/browser.rb index 196f5d65..f537ccae 100644 --- a/lib/ferrum/browser.rb +++ b/lib/ferrum/browser.rb @@ -238,14 +238,23 @@ def restart # # Terminates the browser process and closes the client connection. # - def quit + # @param [Boolean] wait + # Whether to block until the process is confirmed dead and its user + # data directory removed (the default), or return immediately and run + # that cleanup on a background thread instead. See {Process#stop}. + # + # @return [Thread, nil] + # The background cleanup thread when `wait: false`, `nil` otherwise. + # + def quit(wait: true) return unless @client contexts.close_connections @client.close - @process.stop + thread = @process.stop(wait: wait) @client = @process = @contexts = nil + thread end # diff --git a/lib/ferrum/browser/process.rb b/lib/ferrum/browser/process.rb index 50b65a11..df031a53 100644 --- a/lib/ferrum/browser/process.rb +++ b/lib/ferrum/browser/process.rb @@ -8,9 +8,11 @@ require "ferrum/browser/options/base" require "ferrum/browser/options/chrome" require "ferrum/browser/options/firefox" +require "ferrum/browser/process/killer" require "ferrum/browser/command" require "ferrum/utils/elapsed_time" require "ferrum/utils/platform" +require "ferrum/utils/thread" module Ferrum class Browser @@ -21,9 +23,6 @@ class Browser # stopping/restarting the process and cleaning up its user data directory. # class Process - KILL_TIMEOUT = 2 - WAIT_KILLED = 0.05 - extend Forwardable delegate path: :command @@ -40,54 +39,6 @@ def self.start(*args) new(*args).tap(&:start) end - # - # Builds a finalizer proc that kills the process with the given pid. - # - # @param [Integer] pid - # Process id to kill. - # - # @return [Proc] - # - def self.process_killer(pid) - proc do - if Utils::Platform.windows? - # Process.kill is unreliable on Windows - ::Process.kill("KILL", pid) unless system("taskkill /f /t /pid #{pid} >NUL 2>NUL") - else - ::Process.kill("USR1", pid) - start = Utils::ElapsedTime.monotonic_time - while ::Process.wait(pid, ::Process::WNOHANG).nil? - sleep(WAIT_KILLED) - next unless Utils::ElapsedTime.timeout?(start, KILL_TIMEOUT) - - ::Process.kill("KILL", pid) - ::Process.wait(pid) - break - end - end - rescue Errno::ESRCH, Errno::ECHILD - # nop - end - end - - # - # Builds a finalizer proc that removes the directory at the given path. - # - # @param [String] path - # Directory to remove. - # - # @return [Proc] - # - def self.directory_remover(path) - proc { - begin - FileUtils.remove_entry(path) - rescue StandardError - Errno::ENOENT - end - } - end - attr_reader :host, :port, :ws_url, :pid, :command, :default_user_agent, :browser_version, :protocol_version, :v8_version, :webkit_version, :xvfb @@ -108,7 +59,7 @@ def initialize(options) @env = Hash(options.env) tmpdir = Dir.mktmpdir("ferrum_user_data_dir_") - ObjectSpace.define_finalizer(self, self.class.directory_remover(tmpdir)) + ObjectSpace.define_finalizer(self, Killer.directory_remover(tmpdir)) @user_data_dir = tmpdir @command = Command.build(options, tmpdir) end @@ -130,12 +81,12 @@ def start if @command.xvfb? @xvfb = Xvfb.start(@command.options) - ObjectSpace.define_finalizer(self, self.class.process_killer(@xvfb.pid)) + ObjectSpace.define_finalizer(self, Killer.process_killer(@xvfb.pid)) end env = Hash(@xvfb&.to_env).merge(@env) @pid = ::Process.spawn(env, *@command.to_a, process_options) - ObjectSpace.define_finalizer(self, self.class.process_killer(@pid)) + ObjectSpace.define_finalizer(self, Killer.process_killer(@pid)) parse_ws_url(read_io, @process_timeout) parse_json_version(ws_url) @@ -148,17 +99,26 @@ def start # Kills the browser process (and Xvfb, if running) and removes the user # data directory. # - # @return [void] - # - def stop - if @pid - kill(@pid) - kill(@xvfb.pid) if @xvfb&.pid - @pid = nil - end - - remove_user_data_dir if @user_data_dir - ObjectSpace.undefine_finalizer(self) + # @param [Boolean] wait + # Whether to block until the process is confirmed dead and its user + # data directory removed (the default), or return immediately and + # run that cleanup on a background thread instead. Killing a + # stubborn process group can block for up to {Killer::KILL_TIMEOUT} + # seconds, plus retries removing its directory, which matters when quitting + # many browsers in a hot path. `wait: false` is not used by + # {#restart}, which always waits so the old process is fully gone + # before the new one starts. + # + # @return [Thread, nil] + # The background cleanup thread when `wait: false`; the caller can + # `#join` it if they need cleanup to have finished, e.g. before + # process exit or before reusing a fixed port. `nil` when `wait: + # true`. + # + def stop(wait: true) + return sync_stop if wait + + async_stop end # @@ -189,12 +149,40 @@ def inspect private - def kill(pid) - self.class.process_killer(pid).call + def sync_stop + if @pid + Killer.kill(@pid) + Killer.kill(@xvfb.pid) if @xvfb&.pid + @pid = nil + end + + remove_user_data_dir if @user_data_dir + ObjectSpace.undefine_finalizer(self) + nil + end + + # + # Snapshots what needs killing/removing, clears instance state so the + # object looks stopped right away, and does the actual work on a + # background thread. The finalizer is left in place as a backup until + # that thread finishes, in case the process exits before it does. + # + def async_stop + pid = @pid + xvfb_pid = @xvfb&.pid + user_data_dir = @user_data_dir + @pid = @user_data_dir = nil + + Utils::Thread.spawn(abort_on_exception: false) do + Killer.kill(pid) if pid + Killer.kill(xvfb_pid) if xvfb_pid + Killer.remove_directory(user_data_dir) if user_data_dir + ObjectSpace.undefine_finalizer(self) + end end def remove_user_data_dir - self.class.directory_remover(@user_data_dir).call + Killer.remove_directory(@user_data_dir) @user_data_dir = nil end diff --git a/lib/ferrum/browser/process/killer.rb b/lib/ferrum/browser/process/killer.rb new file mode 100644 index 00000000..d41b6788 --- /dev/null +++ b/lib/ferrum/browser/process/killer.rb @@ -0,0 +1,160 @@ +# frozen_string_literal: true + +require "fileutils" +require "ferrum/utils/elapsed_time" +require "ferrum/utils/platform" + +module Ferrum + class Browser + class Process + # + # OS-level process termination and user-data-directory removal, kept + # separate from the rest of {Process} (spawning, CDP handshake, ...). + # Both the GC finalizer and the explicit {Process#stop} path need to + # run the exact same kill/cleanup logic on plain pid/path values -- + # never on `self`. A finalizer proc that closes over the object it's + # attached to creates a reference cycle GC can never collect, so the + # finalizer would never run; keeping this logic as free functions + # taking plain arguments sidesteps that entirely. + # + module Killer + module_function + + KILL_TIMEOUT = 2 + WAIT_KILLED = 0.05 + REMOVE_DIR_RETRIES = 5 + REMOVE_DIR_RETRY_DELAY = 0.1 + + # + # Kills the process at the given pid (and its process group, if + # any), escalating from TERM to KILL if it doesn't exit within + # {KILL_TIMEOUT} seconds. + # + # @param [Integer] pid + # + # @return [void] + # + def kill(pid) + if Utils::Platform.windows? + # Process.kill is unreliable on Windows + ::Process.kill("KILL", pid) unless system("taskkill /f /t /pid #{pid} >NUL 2>NUL") + return + end + + send_signal(pid, "TERM") + start = Utils::ElapsedTime.monotonic_time + leader_exited = false + loop do + # The leader (the pid we actually spawned and can #wait on) may + # exit well before the rest of its process group does -- e.g. a + # renderer/zygote that ignores TERM. Reaping the leader must not + # by itself end the loop, or that child is orphaned forever + # since KILL is only ever sent from the timeout branch below. + 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 + end + rescue Errno::ESRCH, Errno::ECHILD + # nop + end + + # + # Builds a finalizer proc that kills the process with the given pid. + # + # @param [Integer] pid + # Process id to kill. + # + # @return [Proc] + # + def process_killer(pid) + proc { kill(pid) } + end + + # + # Signals the whole process group Chrome was spawned into (it's + # spawned with `pgroup: true`), so its child processes (renderer, + # GPU, zygote, ...) are cleaned up too instead of being left + # orphaned. Falls back to signaling just the pid directly if + # there's no such process group to signal (e.g. Xvfb, which isn't + # spawned with `pgroup: true`) or the group can't be signaled. + # + # @param [Integer] pid + # @param [String] name + # + # @return [void] + # + def send_signal(pid, name) + ::Process.kill(name, -pid) + rescue Errno::EPERM, Errno::ESRCH + ::Process.kill(name, pid) + end + + # + # Checks whether any process in pid's process group is still alive. + # + # @param [Integer] pid + # + # @return [Boolean] + # + def process_group_alive?(pid) + ::Process.kill(0, -pid) + true + rescue Errno::ESRCH + false + rescue Errno::EPERM + true + end + + # + # Removes the given directory, retrying with exponential backoff on + # transient errors. Chrome can briefly hold file locks right after + # being killed, so the directory may not be removable on the first + # try; retrying avoids leaking temp directories in that case. + # + # @param [String] path + # Directory to remove. + # @param [Integer] retries + # Maximum number of removal attempts. + # @param [Float] delay + # Base delay, in seconds, before the first retry; doubles on each + # subsequent attempt. + # + # @return [void] + # + def remove_directory(path, retries: REMOVE_DIR_RETRIES, delay: REMOVE_DIR_RETRY_DELAY) + retries.times do |attempt| + FileUtils.remove_entry(path) + break + rescue Errno::ENOENT + break + rescue Errno::ENOTEMPTY, Errno::EBUSY, Errno::EACCES, Errno::EPERM => e + raise e if attempt == retries - 1 + + sleep(delay * (2**attempt)) + end + rescue StandardError => e + warn("[Ferrum] Failed to remove user data dir #{path}: #{e.class}: #{e.message}") + end + + # + # Builds a finalizer proc that removes the directory at the given + # path. + # + # @param [String] path + # Directory to remove. + # + # @return [Proc] + # + def directory_remover(path) + proc { remove_directory(path) } + end + end + end + end +end diff --git a/sig/ferrum/browser.rbs b/sig/ferrum/browser.rbs index 4ffa780c..411b42aa 100644 --- a/sig/ferrum/browser.rbs +++ b/sig/ferrum/browser.rbs @@ -43,7 +43,7 @@ module Ferrum def restart: () -> void - def quit: () -> void + def quit: (?wait: bool) -> Thread? def crash: () -> Hash[String, untyped] diff --git a/sig/ferrum/browser/process.rbs b/sig/ferrum/browser/process.rbs index 6ffa0cc3..d18ce238 100644 --- a/sig/ferrum/browser/process.rbs +++ b/sig/ferrum/browser/process.rbs @@ -1,10 +1,6 @@ module Ferrum class Browser class Process - KILL_TIMEOUT: ::Integer - - WAIT_KILLED: ::Float - attr_reader host: String? attr_reader port: ::Integer? @@ -47,15 +43,11 @@ module Ferrum def self.start: (Browser::Options options) -> Browser::Process - def self.process_killer: (::Integer pid) -> Proc - - def self.directory_remover: (String path) -> Proc - def initialize: (Browser::Options options) -> void def start: () -> void - def stop: () -> void + def stop: (?wait: bool) -> Thread? def restart: () -> void @@ -63,7 +55,9 @@ module Ferrum private - def kill: (::Integer pid) -> void + def sync_stop: () -> nil + + def async_stop: () -> Thread def remove_user_data_dir: () -> void diff --git a/sig/ferrum/browser/process/killer.rbs b/sig/ferrum/browser/process/killer.rbs new file mode 100644 index 00000000..b410913b --- /dev/null +++ b/sig/ferrum/browser/process/killer.rbs @@ -0,0 +1,27 @@ +module Ferrum + class Browser + class Process + module Killer + KILL_TIMEOUT: ::Integer + + WAIT_KILLED: ::Float + + REMOVE_DIR_RETRIES: ::Integer + + REMOVE_DIR_RETRY_DELAY: ::Float + + def self.kill: (::Integer pid) -> void + + def self.process_killer: (::Integer pid) -> Proc + + def self.send_signal: (::Integer pid, ::String name) -> void + + def self.process_group_alive?: (::Integer pid) -> bool + + def self.remove_directory: (::String path, ?retries: ::Integer, ?delay: ::Float) -> void + + def self.directory_remover: (::String path) -> Proc + end + end + end +end diff --git a/spec/browser_spec.rb b/spec/browser_spec.rb index 139ebc55..a2c4d276 100644 --- a/spec/browser_spec.rb +++ b/spec/browser_spec.rb @@ -379,6 +379,36 @@ expect { Process.kill(0, pid) }.to raise_error(Errno::ESRCH) end + + it "returns immediately with wait: false, finishing cleanup once the returned thread is joined", skip: Ferrum::Utils::Platform.windows? do + browser = Ferrum::Browser.new + pid = browser.process.pid + + start = Ferrum::Utils::ElapsedTime.monotonic_time + thread = browser.quit(wait: false) + elapsed = Ferrum::Utils::ElapsedTime.monotonic_time - start + + expect(thread).to be_a(Thread) + expect(elapsed).to be < 1 + + thread.join + + expect { Process.kill(0, pid) }.to raise_error(Errno::ESRCH) + end + end + + describe "#restart" do + it "blocks until the old process is confirmed dead before returning", skip: Ferrum::Utils::Platform.windows? do + browser = Ferrum::Browser.new + old_pid = browser.process.pid + + browser.restart + + expect { Process.kill(0, old_pid) }.to raise_error(Errno::ESRCH) + expect(browser.process.pid).not_to eq(old_pid) + ensure + browser&.quit + end end describe "#resize" do diff --git a/spec/unit/process/killer_spec.rb b/spec/unit/process/killer_spec.rb new file mode 100644 index 00000000..50e805fb --- /dev/null +++ b/spec/unit/process/killer_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require "tmpdir" + +describe Ferrum::Browser::Process::Killer do + describe ".remove_directory" do + it "removes an existing directory" do + dir = Dir.mktmpdir + + described_class.remove_directory(dir) + + expect(Dir.exist?(dir)).to be false + end + + it "treats an already-gone directory as success, without warning" do + dir = Dir.mktmpdir + FileUtils.remove_entry(dir) + + expect(described_class).not_to receive(:warn) + expect { described_class.remove_directory(dir) }.not_to raise_error + end + + it "retries a transient error and succeeds once it clears" do + dir = Dir.mktmpdir + attempts = 0 + allow(FileUtils).to receive(:remove_entry).and_wrap_original do |original, path| + attempts += 1 + raise Errno::ENOTEMPTY, path if attempts == 1 + + original.call(path) + end + + described_class.remove_directory(dir, retries: 3, delay: 0.001) + + expect(attempts).to eq(2) + expect(Dir.exist?(dir)).to be false + end + + it "warns and gives up without raising once retries are exhausted" do + dir = Dir.mktmpdir + allow(FileUtils).to receive(:remove_entry).and_raise(Errno::EBUSY) + + expect(described_class).to receive(:warn).with(a_string_matching(/Errno::EBUSY/)) + expect { described_class.remove_directory(dir, retries: 2, delay: 0.001) }.not_to raise_error + ensure + Dir.rmdir(dir) + end + + it "warns instead of raising on an error it doesn't specifically retry" do + dir = Dir.mktmpdir + allow(FileUtils).to receive(:remove_entry).and_raise(ArgumentError, "boom") + + expect(described_class).to receive(:warn).with(a_string_matching(/ArgumentError: boom/)) + expect { described_class.remove_directory(dir) }.not_to raise_error + ensure + Dir.rmdir(dir) + end + end + + describe ".process_group_alive?", if: Ferrum::Utils::Platform.mri? do + it "returns true while the process group has a live member" do + pid = Process.spawn("sleep 30", pgroup: true) + + begin + expect(described_class.process_group_alive?(pid)).to be true + ensure + Process.kill("KILL", -pid) + Process.wait(pid) + end + end + + it "returns false once the process group is gone" do + pid = Process.spawn("true", pgroup: true) + Process.wait(pid) + + expect(described_class.process_group_alive?(pid)).to be false + end + end + + describe ".process_killer" do + it "builds a proc that kills the given pid when called", if: Ferrum::Utils::Platform.mri? do + pid = Process.spawn("sleep 30", pgroup: true) + expect(described_class).to receive(:kill).with(pid) + + described_class.process_killer(pid).call + ensure + Process.kill("KILL", -pid) + Process.wait(pid) + end + end + + describe ".directory_remover" do + it "builds a proc that removes the given directory when called" do + dir = Dir.mktmpdir + + described_class.directory_remover(dir).call + + expect(Dir.exist?(dir)).to be false + end + end +end diff --git a/spec/unit/process_spec.rb b/spec/unit/process_spec.rb index 5ad6c435..5187c66e 100644 --- a/spec/unit/process_spec.rb +++ b/spec/unit/process_spec.rb @@ -14,11 +14,78 @@ subject.send(:start) - expect(Process).to receive(:kill).with("USR1", 5678).ordered + expect(Process).to receive(:kill).with("TERM", -5678).ordered + expect(Process).to receive(:kill).with("KILL", -5678).ordered + + subject.quit + end + + it "falls back to signaling the leader pid when the group can't be signaled" do + allow(Process).to receive(:spawn).and_return(5678) + allow(Process).to receive(:wait).and_return(nil) + allow(Ferrum::Client).to receive(:new).and_return(double.as_null_object) + + allow_any_instance_of(Ferrum::Browser::Process).to receive(:parse_ws_url) + allow_any_instance_of(Ferrum::Browser::Process).to receive(:parse_json_version) + + subject.send(:start) + + allow(Process).to receive(:kill).with("TERM", -5678).and_raise(Errno::EPERM) + expect(Process).to receive(:kill).with("TERM", 5678).ordered + allow(Process).to receive(:kill).with("KILL", -5678).and_raise(Errno::EPERM) expect(Process).to receive(:kill).with("KILL", 5678).ordered subject.quit end + + it "falls back to signaling the pid directly when it isn't a process group leader" do + # e.g. Xvfb, which is spawned without `pgroup: true` + allow(Process).to receive(:spawn).and_return(5678) + allow(Process).to receive(:wait).and_return(nil) + allow(Ferrum::Client).to receive(:new).and_return(double.as_null_object) + + allow_any_instance_of(Ferrum::Browser::Process).to receive(:parse_ws_url) + allow_any_instance_of(Ferrum::Browser::Process).to receive(:parse_json_version) + + subject.send(:start) + + allow(Process).to receive(:kill).with("TERM", -5678).and_raise(Errno::ESRCH) + expect(Process).to receive(:kill).with("TERM", 5678).ordered + allow(Process).to receive(:kill).with("KILL", -5678).and_raise(Errno::ESRCH) + expect(Process).to receive(:kill).with("KILL", 5678).ordered + + subject.quit + end + + it "kills the whole process group, not just the leader, when a group member ignores TERM", if: Ferrum::Utils::Platform.mri? do + child_pid_path = File.join(Dir.tmpdir, "ferrum-process-spec-child-pid-#{Process.pid}") + + script = <<~RUBY + pid = fork do + trap("TERM", "IGNORE") + sleep 30 + end + Process.detach(pid) + File.write(#{child_pid_path.inspect}, pid.to_s) + sleep 30 + RUBY + leader_pid = Process.spawn(RbConfig.ruby, "-e", script, pgroup: true) + + begin + start = Ferrum::Utils::ElapsedTime.monotonic_time + sleep(0.05) until File.size?(child_pid_path) || Ferrum::Utils::ElapsedTime.timeout?(start, 5) + child_pid = File.read(child_pid_path).to_i + + # A leader that dies promptly on TERM must not short-circuit the + # escalation to KILL for the rest of its process group. + Ferrum::Browser::Process::Killer.kill(leader_pid) + sleep(0.2) + + expect { Process.kill(0, child_pid) }.to raise_error(Errno::ESRCH) + ensure + File.delete(child_pid_path) if child_pid && File.exist?(child_pid_path) + end + end end context "env variables" do