Skip to content

handler: bound each Page command by HandlerConfig::request_timeout - #336

Open
URootX wants to merge 1 commit into
mattsse:mainfrom
URootX:pr/configurable-request-timeout
Open

handler: bound each Page command by HandlerConfig::request_timeout#336
URootX wants to merge 1 commit into
mattsse:mainfrom
URootX:pr/configurable-request-timeout

Conversation

@URootX

@URootX URootX commented Sep 2, 2026

Copy link
Copy Markdown

Problem

CommandFuture::new starts its timer from the hard-coded handler::REQUEST_TIMEOUT (30 s) regardless of what the caller put in HandlerConfig::request_timeout. The configured value only drives the periodic eviction of unanswered commands, so a Page::execute that Chrome legitimately answers after 30 s always fails with CdpError::Timeout — for example a Runtime.evaluate with awaitPromise: true whose promise settles at 35 s, even with request_timeout set to minutes.

Fix

Thread the per-target request_timeout (already carried in TargetConfig) through PageHandle::newPageInnerCommandFuture::new, and use it for the futures_timer::Delay. REQUEST_TIMEOUT stays as the default, so behaviour is unchanged for callers who never touch the config.

  • src/handler/commandfuture.rs: CommandFuture::new takes request_timeout: Duration.
  • src/handler/page.rs: PageHandle::new takes it and stores it on PageInner; command_future passes it on.
  • src/handler/target.rs: pass self.config.request_timeout when the page handle is created.
  • CHANGELOG entry under Unreleased.

Browser::execute (browser-level commands) is untouched — it has no per-command timer today.

Verification

cargo check / cargo test -p chromiumoxide on this branch. Exercised end to end from a daemon that sets request_timeout to 180 s: a Runtime.evaluate that takes 40 s now returns its value; on main the same call fails at 30 s with Request timed out.

CommandFuture hard-coded REQUEST_TIMEOUT (30 s) regardless of the configured
request_timeout, which only drove eviction. Thread the per-target value through
PageHandle -> PageInner -> CommandFuture::new so a caller that configures a
longer timeout actually gets it (e.g. Runtime.evaluate awaiting a promise that
settles after 30 s). Default behaviour is unchanged.

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant