crawlee icon indicating copy to clipboard operation
crawlee copied to clipboard

Refactor timeout handling

Open B4nan opened this issue 8 months ago • 3 comments

Which package is the feature request for? If unsure which one to select, leave blank

None

Feature

Currently, we have two main options to control timeouts of the request handler:

  • navigationTimeoutSecs for making the request (opening a page, or firing an HTTP request)
  • requestHandlerTimeoutSecs for the request handler itself (user-defined function)

Due to how things are implemented in both HTTP crawlers and browser crawlers, the navigation (including navigation hooks) is executed inside the time window of the request handler. We also add a 10s "buffer" to accommodate internal code and the navigation hooks.

This can be very confusing, because a user sees a 60s request handler timeout, but the errors will mention 130s (or 100s for HTTP crawlers where navigation timeout is only 30s) instead, since we sum those two timeouts together, and add a magic 10s buffer that is not really documented anywhere.

We should refactor things so that the request handler timeout is only applied to the actual request handler function provided by the user. We already handle the navigation timeout specifically for the navigation.

Motivation

.

Ideal solution or implementation, and any additional constraints

.

Alternative solutions or implementations

No response

Other context

We should also consider introducing a new timeout option for the navigation hooks, or keep them as part of the navigation timeout. Currently, we pass this timeout to the underlying library (playwright/puppeteer/got-scraping/...), and only the main "combined" timeout is now applied. We probably want to keep this combined timeout too, in case some parts of our internal code get stuck (e.g. API calls), but the error message for it should clearly state the logic behind this number, and it shouldn't be called "requestHandler timeout" in the first place. We already have an internal timeout so maybe we could reuse that naming here.

https://apify.slack.com/archives/C0L33UM7Z/p1745846410237329

B4nan avatar Apr 30 '25 10:04 B4nan

Working on this with @ezequiel38

anghel9 avatar Jul 17 '25 19:07 anghel9

Note that we consider those changes breaking; they are planned for the next major version. We already have a branch for that: https://github.com/apify/crawlee/tree/v4

B4nan avatar Jul 17 '25 20:07 B4nan

After some testing and exploring the codebase, we have a few ideas and questions.

You included a link at the end, but we aren't able to view it. What timeout naming did you want us to use? I found an internal timeout in the code that is already being used, is that the one that you're referring to in the link? We want to rename this line that gets called when the combined timeout is reached.

You mentioned that the navigation timeout is already handled specifically for navigation, which is this file that is called here. Should we use a similar structure for the user request handler?

We did some tests and found out that the navigations hooks are not included in the _handleNavigationTimeout. So if the hooks are slow they can cause the combined timeout to be triggered which could lead to some confusion. Did you want us to incorporate it into the _handleNavigationTimeout or create separate timeout for the hooks, if so how long?

ezequiel38 avatar Aug 04 '25 20:08 ezequiel38