fix: Only apply requestHandlerTimeout to request handler

[janbuchar]

I stumbled upon this when working on ContextPipeline for the JS version. I'm eager to hear your thoughts 🙂

[vdusek]

Okay, so the requestHandlerTimeout is now applied only to the request handler (router(final_context)) and not the whole context pipeline.

It makes sense when I'm reading this.

Do we have any examples of where the previous behavior caused any troubles?

[janbuchar]

Okay, so the requestHandlerTimeout is now applied only to the request handler (router(final_context)) and not the whole context pipeline.

This is correct.

Do we have any examples of where the previous behavior caused any troubles?

In the JS version, the browser crawler (playwright/puppeteer ancestor) has two kinds of timeout - navigationTimeout and requestHandlerTimeout. Then it does this: https://github.com/apify/crawlee/blob/master/packages/browser-crawler/src/internals/browser-crawler.ts#L395

This is super awkward and not really what the interface promises. However, the Python version doesn't have a navigation timeout. I believe I should add that as part of this PR.

[Pijukatel]

Could you please add some test for the correct application of the timeout.

[vdusek]

Docstring suggestions. Otherwise LGTM.

[Pijukatel]

I am not sure about this. Context is expanded by many pipeline steps, so this mapping is not very flexible as it will break if you try to apply SharedTimeout over pipeline steps that work on expanded context.

Maybe it would be better to keep the timeout on the context itself, instead of this mapping on the crawler? I think it belongs there also conceptually. The crawler sets the timeout, but it belongs to the context. Also only the consumers of this specific context should have access to the timeout, there is no reason to keep it global for the whole Crawler.

[janbuchar]

Same here 🙂 I want to avoid exposing the timeout to the request handler, mostly because it just doesn't make sense to me to do so.

Since the tests are now failing because of this, I guess we can agree that this approach is not optimal - I'll iterate on it for a while.

[Pijukatel]

Nice idea. I like this a lot.

Just thinking out loud, where this could lead to:
In case we need to create more granular timeouts for specific steps, I think this class could be easily expanded to support that. I am wondering if even the final context consumer (request handler) could just use timeout from here if the timeout is set on the context (my other comment)

[janbuchar]

I'm not sure I follow, do you think that the request handler should be limited by a shared timeout? Or that it should have access to the remaining timeout "budget"?

[Pijukatel]

Please do not focus on any specific interface example in my example. It is just about capability.

Imagine that the context would be created something like this:

pipeline_timeout = SharedTimeout(...)

BasicCrawlingContext(.....,
 timeouts={
"WholePipeline": pipeline_timeout,   # Maybe the other timeouts could be somehow limited by this one? 
"Navigation": pipeline_timeout.limited_to(NAVIGATION_LIMIT),
"RequestHandler": pipeline_timeout.limited_to(HANDLER_LIMIT)
})

And each timeout-protected context consumer would pick the relevant timeout from the context and apply it. Context consumers could even modify the timeouts of the steps that follow them.

For example, users could mutate "RequestHandler" timeout in pre-navigation hooks.

[Pijukatel]

No need to block this change for this. If needed, we can discuss here: #1596

[Mantisus]

An excellent solution with context for a general navigation timeout!