change(options): stop tying :protocol_timeout to :timeout - #633
Closed
route wants to merge 1 commit into
Closed
Conversation
`:protocol_timeout` bounds internal CDP bookkeeping, `Target.createTarget`, `Target.attachToTarget` and friends, which resolve in milliseconds. It defaulted to `:timeout`, so anyone lowering that to keep page waits short also gave the browser's own bookkeeping the same tiny budget, and on a loaded machine page creation started raising `Ferrum::TimeoutError`. It now defaults to 30 seconds, still overridable per browser or through `FERRUM_PROTOCOL_TIMEOUT`. Running out of it means the machine is struggling rather than the browser being gone, which is detected on its own. Refs #470
Member
Author
|
Closing this. 30s is far too much for a protocol call: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #470.
The problem
:timeoutis a user-facing knob: how long to wait for a navigation, an evaluation, a DOM query. People set it low on purpose.:protocol_timeoutis a different thing, the budget for internal CDP bookkeeping,Target.createTarget,Target.attachToTarget,Page.enable, calls that resolve in milliseconds on a healthy machine.Tying the second to the first means
Ferrum::Browser.new(timeout: 1)also gives target creation one second, and 5 by default. On a loaded CI box that is not enough, and what comes out is aFerrum::TimeoutErrorwhile creating a page, which is precisely the shape of #470:Page.enablenever answering on a busy runner, then the whole run unravelling.The change
:protocol_timeoutdefaults to 30 seconds, independent of:timeout, still settable per browser and throughFERRUM_PROTOCOL_TIMEOUT. Page-level waits are unaffected,Page#commandpasses:timeoutas before.Generous is the right default here: running out of this budget means the machine is struggling, not that the browser is gone. A browser that has actually died is detected by the socket closing, not by this timer.
Verification
spec/browser_spec.rb:timeout: 1no longer dragsprotocol_timeoutdown with it, and the setter still works. Fails before the change,protocol_timeoutis 1 there.:protocol_timeoutis now documented indocs/2-customization.mdnext to:timeout, it was missing.Best merged after #630, which is what makes a dead browser raise immediately rather than after this timeout. On its own this change makes that detection slower, 30s instead of 5s, until #630 lands.