#2134 fetcher.thread.timeout: cancel the okhttp call, bounded helper pool for other protocols

[GGraziadei]

Design doc available here https://docs.google.com/document/d/1XXwdz8-FQaRurvSpljVUdE3IuRmj-lG6/edit?usp=sharing&ouid=114888462700939773114&rtpof=true&sd=true

Fixes #2134.

fetcher.thread.timeout ran the protocol call on a single-thread executor owned by each FetcherThread and abandoned it with future.cancel(true) when the deadline passed. okhttp does not honour the interrupt while connecting or reading, so the helper stayed blocked on the socket and the FetcherThread queued behind it at its next fetch: one host dribbling bytes could take a thread out of service for as long as http.timeout. The option also doubled the bolt's thread count, and the robots.txt lookup was not covered by the timeout at all.

Change

okhttp protocol

• fetcher.thread.timeout is applied as a per-call deadline with Call.timeout(), enforced by okio's shared watchdog thread: on expiry the call is cancelled, the socket closed and the fetching thread gets an InterruptedIOException immediately, classified as "Socket timeout fetching" as before.
• The deadline is clamped to topology.message.timeout.secs, so that it can never loosen the client-level callTimeout derived from it (a warning is logged).
• With http.content.partial.as.trimmed the content received before the deadline is kept and flagged as trimmed for "time", which is what that option already did for the call timeout. Documented.
• Protocol gains default boolean supportsFetchTimeout() (false), backward compatible for external and user protocols. okhttp returns true when the deadline is configured; DelegatorProtocol only when every delegate does, since the bolt cannot know in advance which delegate a URL is routed to.

Fetcher bolts

• New package-private FetchTimeoutHelpers, owned by FetcherBolt and SimpleFetcherBolt. Its call() runs both the robots.txt lookup and the fetch: on the calling thread when the protocol enforces the timeout itself, otherwise on a helper thread from one bounded pool per bolt (fetcher.thread.timeout.helpers, default 2 × fetcher.threads.number, 2 for SimpleFetcherBolt; threads created on demand, released after a minute idle).
• A deadline on the helper path throws a typed TimeoutException; a full pool rejects at once with SaturatedException, reported as FETCH_ERROR with fetch.exception "No fetch helper available". Helpers are shared by all hosts, so a host that never answers can make fetches of other hosts fail this way until its helpers time out; stated in the docs.
• With the default okhttp protocol no helper thread is ever created.
• New fetchhelpers gauge and fetch.timeout / fetch.helper.rejected counters.
• fetcher.thread.timeout is now defined in Constants; the public alias FetcherBolt.FETCH_TIMEOUT_PARAM_KEY (Add bolt-level timeout for fetcher threads #1861) is removed. The configuration key is unchanged.
• configuration.adoc documents fetcher.thread.timeout, which was missing, and the new helpers key.

Behaviour

protocol	fetcher.thread.timeout set	before	after
okhttp (default)	yes	fetch abandoned on a per-thread helper that stays blocked; 50 extra threads	call cancelled, socket closed, no helper threads
okhttp	no	unchanged	unchanged
other (Playwright, custom)	yes	per-thread helper; a stuck helper pinned its FetcherThread	bounded shared pool; stuck helpers do not pin threads; explicit saturation error
any	yes	robots.txt lookup outside the timeout	covered

Exception classification now also maps InterruptedIOException (okhttp's own timeouts) to "Socket timeout fetching" regardless of the option; previously such failures carried the exception class name in fetch.exception.

Tests

• HttpProtocolFetchTimeoutTest: a 5s response is cancelled at the 1s deadline; a 60s deadline is clamped to a 1s message timeout; partial content kept or failed depending on http.content.partial.as.trimmed.
• FetchTimeoutHelpersTest: every branch of call() (off, self-timing protocol, helper thread, exception propagation, typed timeout with interrupt, saturation, bound from config, shutdown).
• Fetcher bolt tests, run for both bolts, with a test protocol that hangs and ignores interruption: a stuck fetch followed by two fast ones on one fetcher thread yields two pages and one FETCH_ERROR (before: all three timed out); bounded pool with one rejection; hanging robots.txt reported at the timeout (before: the thread blocked for good).
• FetcherBoltTest: no helper threads with okhttp; slow robots.txt bounded by the deadline with okhttp.
• The existing testThreadTimeout passes unchanged through the okhttp path.

---

For all changes

• Is there a issue associated with this PR? Is it referenced in the commit message?
• Does your PR title start with #XXXX where XXXX is the issue number you are trying to resolve?
• Has your PR been rebased against the latest commit within the target branch (typically main)?
• Is your initial contribution a single, squashed commit?
• Is the code properly formatted with mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?

For code changes

• Have you ensured that the full suite of tests is executed via mvn clean verify?
• Have you written or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0? (no new dependencies)
• If applicable, have you updated the LICENSE file, including the main LICENSE file? (not applicable)
• If applicable, have you updated the NOTICE file, including the main NOTICE file? (not applicable)

[GGraziadei]

Moving on DRAFT
Pending discussion and design doc on dev@

[rzo1]

Thanks for picking this up. A few things from reading the diff:

robots.txt timeout behaves differently per protocol

With okhttp the deadline fires inside http.getProtocolOutput(), and HttpRobotRulesParser catches that (the catch (Throwable t) around line 218). It returns EMPTY_RULES and puts them in the error cache, so the page gets fetched as if there were no robots.txt. On the helper path the same timeout ends up as FETCH_ERROR for the URL. The okhttp behaviour isn't new, but the yaml comment and the PR description read as if both paths handle a hanging robots.txt the same way. I'd make them consistent.

The deadline is per call, not per fetch, with okhttp

The robots lookup can follow up to 5 redirects and each one gets the full timeout, then the page fetch gets another one. Worst case that's around 7x fetcher.thread.timeout for one URL. Fine by me if it's intended, but the docs should say so, because "hard timeout for a single fetch" suggests otherwise.

DelegatorProtocol.supportsFetchTimeout()

If any delegate returns false, every URL goes to the helper pool, including the ones routed to okhttp. The delegate is chosen per URL anyway, so could we resolve it first, e.g. supportsFetchTimeout(url, metadata), or check getProtocolFor(url, metadata) in the bolt?

fetch.timeout counter

It counts any InterruptedIOException, and SocketTimeoutException is a subclass, so normal http.timeout socket timeouts are counted too. That makes it hard to see how often the new deadline actually fires. Could the deadline get its own counter?

Clamping

The clamp to topology.message.timeout.secs is only in HttpProtocol. Doing it in FetchTimeoutHelpers would cover both paths.

Nit

StuckProtocol.HANG_ROBOTS is static mutable state shared between tests. OK while tests run serially, but it will break with parallel surefire.

[dpol1]

Two small things on top of rzo1's points.

Findings flagged by Claude

[dpol1]

FetcherBolt.FETCH_TIMEOUT_PARAM_KEY shipped in 3.6.0 and 3.7.0, so code that used it stops compiling. Keep a deprecated alias, or note it for 4.0?

[GGraziadei]

Kept FetcherBolt.FETCH_TIMEOUT_PARAM_KEY as a deprecated alias of Constants.FETCH_TIMEOUT_PARAM_KEY, marked for removal, so existing code keeps compiling.

[dpol1]

Grazie Gianluca, the alias does it. Only since should read 4.0, 3.8 will not exist. Necessary since this is a public API

[GGraziadei]

Thanks Davide. I am fixing it :-) ;

[GGraziadei]

Rebased on main (#2128 changed the redirect handling in the same code) and addressed the points:

robots.txt timeout per path: now the same on both. A lookup which times out on a helper thread lets the page be fetched without rules, which is what HttpRobotRulesParser already does with okhttp when the lookup fails. Counted by a new robots.timeout counter. Test hangingRobotsLookupDoesNotFailTheUrl checks that the fetch is attempted after the lookup timed out.

Per call vs per fetch: with the manual redirect following from #2128 the page fetch now shares one deadline across its hops, each hop getting the time left (redirectChainSharesTheDeadline). The robots.txt lookup and each redirect it follows are still calls of their own with their own deadline; the yaml comment and configuration.adoc say so, including the worst case.

DelegatorProtocol: the capability is supportsFetchTimeout(url, metadata) and the delegator resolves the delegate per URL. Since the robots.txt lookup is routed with its own metadata, the delegate on that route must support it too, otherwise the lookup would run inline on a protocol which cannot cancel it (supportsFetchTimeoutRequiresTheRobotsDelegateToo).

Counter: okhttp throws a typed FetchTimeoutException (an InterruptedIOException) when the watchdog cancelled the call, and the helper path throws the same. fetch.deadline counts only those; fetch.timeout keeps counting every timeout, http.timeout included. socketTimeoutIsNotTheDeadline covers the distinction.

Clamping: moved to FetchTimeout.secs(conf), used by both HttpProtocol and FetchTimeoutHelpers, with a test on the helper side.

StuckProtocol: HANG_ROBOTS is a configuration key read in configure(), per instance.

[dpol1]

Second round looks good; two questions inline.

Findings flagged by Claude

[dpol1]

With asap = true a queue with more URLs is eligible again immediately while the helper pool is still full, so it collects repeated rejections and WARNs. Could saturation get some backoff?

[GGraziadei]

Good catch, thanks! I am checking something like exponential backoff with equal jitter implementation.

[dpol1]

DNS can still hold the fetcher thread past this deadline: cancellation cannot release the synchronous lookup until the resolver returns. Could we say so next to the hard-deadline description?

[rzo1]

Thanks, the cancellation approach looks good. Two things:

• Pool rejects while a helper is idle (FetchTimeoutHelpers): with ThreadPoolExecutor(0, max, SynchronousQueue), future.get() returns before the worker is back in poll(). The page fetch submitted right after the robots lookup can then get SaturatedException when the pool is at max. A standalone repro with max=2 and one stuck task rejected ~1% of URLs, and abandonedFetchesUseABoundedSharedPool fails intermittently because of it. A Semaphore (tryAcquire before submit, release in finally) in front of a queueing executor would avoid this.
• Timed-out robots lookups aren't cached (FetcherBolt:734, SimpleFetcherBolt:302): the helper path only uses EMPTY_RULES locally. Every later URL for the host starts another robots.txt request on a new helper and waits a full deadline. Could the timeout result go into the error cache like other failed lookups?