diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f477cd3..7545ea69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,9 @@ the browser; a failed `IO.close` is raised to the caller. ### Fixed +- Commands in flight when the browser died waited out `:protocol_timeout` each before raising, so a dead browser + turned into a long parade of slow failures. They're released as soon as the socket closes, and a command sent + after that raises `Ferrum::DeadBrowserError` right away [#470] - A raising callback killed `Client::Subscriber`'s dispatch thread and with it every event for the rest of the process: pages created later raised `NoSuchTargetError`, open ones lost their execution contexts. Callbacks are now rescued one by one and reported to stderr [#470] diff --git a/lib/ferrum/client.rb b/lib/ferrum/client.rb index f6dd0045..4e1c8716 100644 --- a/lib/ferrum/client.rb +++ b/lib/ferrum/client.rb @@ -219,6 +219,8 @@ def send_message(message, async:, timeout: nil) @ws.send_message(message) true else + raise DeadBrowserError if @ws.messages.closed? + pending = Concurrent::IVar.new @pendings[message[:id]] = pending @ws.send_message(message) @@ -340,9 +342,17 @@ def start @pendings[message["id"]]&.set(message) end end + + release_pendings end end + # Nothing is going to answer the commands still in flight once the socket + # is gone, release them instead of making each wait out its timeout. + def release_pendings + @pendings.each_value { |pending| pending.try_set(nil) } + end + # Locked so two concurrent commands never share an id and read each # other's responses from @pendings. def next_command_id diff --git a/sig/ferrum/client.rbs b/sig/ferrum/client.rbs index d75549a0..bc90a592 100644 --- a/sig/ferrum/client.rbs +++ b/sig/ferrum/client.rbs @@ -74,6 +74,8 @@ module Ferrum def start: () -> void + def release_pendings: () -> void + def next_command_id: () -> ::Integer def raise_browser_error: (Hash[String, untyped] error) -> void diff --git a/spec/client_spec.rb b/spec/client_spec.rb index 34e3de39..2788c647 100644 --- a/spec/client_spec.rb +++ b/spec/client_spec.rb @@ -1,6 +1,37 @@ # frozen_string_literal: true describe Ferrum::Client do + describe "a dead connection" do + let(:remote) { Ferrum::Browser.new(base_url: base_url, timeout: 10, protocol_timeout: 10) } + + after { remote.quit } + + def messages + remote.client.instance_variable_get(:@ws).messages + end + + it "releases the commands in flight" do + page = remote.create_page + Thread.new do + sleep(0.2) + messages.close + end + start = Ferrum::Utils::ElapsedTime.monotonic_time + + expect { page.go_to("/really_slow") }.to raise_error(Ferrum::DeadBrowserError) + expect(Ferrum::Utils::ElapsedTime.elapsed_time(start)).to be < 5 + end + + it "fails the commands that follow without waiting" do + page = remote.create_page + messages.close + start = Ferrum::Utils::ElapsedTime.monotonic_time + + expect { page.go_to("/") }.to raise_error(Ferrum::DeadBrowserError) + expect(Ferrum::Utils::ElapsedTime.elapsed_time(start)).to be < 1 + end + end + describe "event callbacks" do let(:remote) { Ferrum::Browser.new(base_url: base_url, timeout: 3, protocol_timeout: 3) }