Skip to content
This repository has been archived by the owner on Nov 23, 2023. It is now read-only.

Add pending logging #5

Open
wants to merge 2 commits into
base: freeagent-logging-branch
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/ferrum/browser/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def command(method, params = {})
@pendings.delete(message[:id])

raise DeadBrowserError if data.nil? && @ws.messages.closed?
raise TimeoutError unless data
raise TimeoutError.new() unless data

error, response = data.values_at("error", "result")
raise_browser_error(error) if error
Expand Down
16 changes: 11 additions & 5 deletions lib/ferrum/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,17 @@ def initialize(url, pendings = [])
end

class TimeoutError < Error
def message
"Timed out waiting for response. It's possible that this happened " \
"because something took a very long time (for example a page load " \
"was slow). If so, setting the :timeout option to a higher value might " \
"help."
attr_reader :pending_urls

def initialize(pending_urls = [])
@pending_urls = pending_urls

message = "Timed out waiting for response. It's possible that this happened " \
"because something took a very long time (for example a page load " \
"was slow). If so, setting the :timeout option to a higher value might " \
"help."
message = "#{message}\n Connections still pending:\n #{pending_urls.join(', ')}" unless pending_urls.empty?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a critique of your code here (because I use unless a lot) - but I read this the other day:
Read This Post 'Unless' You're Not A Ruby Developer

https://news.ycombinator.com/item?id=33965933

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to override the whole message string, or just append to it if there are pending urls? (it's replacing it atm)

Copy link

@singhprd singhprd Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like: (I think that's how you add to a string.. )

Suggested change
message = "#{message}\n Connections still pending:\n #{pending_urls.join(', ')}" unless pending_urls.empty?
if !pending_urls.empty?
message =+ "#{message}\n Connections still pending:\n #{pending_urls.join(', ')}" unless pending_urls.empty?
end

super(message)
end
end

Expand Down
9 changes: 8 additions & 1 deletion lib/ferrum/network.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,14 @@ def wait_for_idle(connections: 0, duration: 0.05, timeout: @page.browser.timeout
start = Utils::ElapsedTime.monotonic_time

until idle?(connections)
raise TimeoutError if Utils::ElapsedTime.timeout?(start, timeout)
if Utils::ElapsedTime.timeout?(start, timeout)
if @page.browser.options.pending_connection_errors
pendings = traffic.select(&:pending?)
pending_urls = pendings.map(&:url).compact

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the pending connection always have a url here? I thought we saw that sometimes it was empty, because the pendings are a collection of Network Exchange objects, from which the url method is request&.url. I could be wrong here - and it could also be that when we tested last time the exchange didn't have a request (and therefore no url) but when it's run normally they do.

puts("[DEBUG] 'wait_for_idle' pending connections:\n#{pendings.map(&:inspect)}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea 👌

end
raise TimeoutError.new(pending_urls = pending_urls)
end

sleep(duration)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/ferrum/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ def command(method, wait: 0, slowmoable: false, **params)
@event.wait(wait)
if iteration != @event.iteration
set = @event.wait(timeout)
raise TimeoutError unless set
raise TimeoutError.new() unless set
end
end
result
Expand Down