Synchronous DNS resolution can wedge a crawl past --max-time#613
Merged
Conversation
host_wait() is a stub returning 1, so a STATUS_WAIT_DNS slot resolves synchronously through hts_dns_resolve_all() -> getaddrinfo() on the engine thread. Nothing bounded that call, and it ran holding opt->state.lock, so a black-hole resolver stalled the crawl past --max-time and --timeout and blocked hts_request_stop()/hts_has_stopped() with it. resolv.conf caps getaddrinfo on POSIX and hides this most of the time; Windows has no equivalent. Resolve a cache miss on a worker thread and wait at most opt->timeout for it, with the lock held only for the cache lookup and the store. A timed-out resolve is abandoned rather than cancelled (getaddrinfo has no portable cancellation), so the job is refcounted and the worker touches nothing but it. A timeout is reported as "does not resolve" but never cached, so the host is retried instead of written off for the rest of the crawl. This bounds the wedge; it does not make resolution asynchronous. The engine thread still stalls other slots for up to --timeout per resolve, which only driving STATUS_WAIT_DNS from the poll loop would fix. New -#test=dnstimeout self-test drives a mock resolver that blocks for 3s past a 1s --timeout; all five of its checks fail on master. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
…kers The self-test's ceiling came from the mock's sleep (MOCK_SLOW_MS - 500), not from opt->timeout: a resolve using a hardcoded 2400ms deadline that ignored opt->timeout entirely still passed it. Derive the bound from opt->timeout so the assertion tests the contract instead of the fixture. The scaffolding also raced its own abandoned workers. mock_host.calls was read while a worker was still writing it, and tearing the backend down at the end raced the worker's freeaddrinfo read, ordered only by a sleep. Serialize the counters under a mutex, wait for the backend calls to actually return instead of guessing, and leave the backend installed, since an abandoned worker still reads it. Drops the drain sleep: 6.0s -> 4.0s. Also: --timeout now bounds resolution, so say so in the help text (the man page is generated from it), and factor the three address-copy loops into a helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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
1 participant
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.
host_wait()is a stub returning 1, so aSTATUS_WAIT_DNSslot resolves synchronously throughgetaddrinfo()on the engine thread while holdingopt->state.lock. A slow resolver stalls the crawl past--max-timeand--timeout, and blockshts_request_stop()behind it. The OS resolver imposes its own cap, which is why this usually reads as a pause rather than a hang: Windows around 10s, Linux'sresolv.confdefaults (5s, two attempts, per nameserver) up to roughly 40s. Neither cap is--timeout, and neither is ours to set.A cache miss now resolves on a worker thread bounded by
opt->timeout, with the lock held only for the cache lookup and the store. The awkward part is the abandoned resolve:getaddrinfohas no portable cancellation, so a timed-out worker keeps running and has to be safe to leave behind. The job is refcounted, and the worker writes only the job, neveropt(freed before the thread wait at exit) or the DNS cache. A timeout is reported as "does not resolve" but deliberately not cached, so a transient stall does not write the host off for the whole crawl.This bounds the wedge rather than closing #606, in two ways. Resolution is still synchronous, so the engine thread still stalls other slots for up to
--timeoutper resolve; only drivingSTATUS_WAIT_DNSfrom the poll loop fixes that. And an abandoned worker still counts as a live thread, sohtsthread_wait()at exit waits out the original resolve: the crawl carries on meanwhile and exit is never later than before, but a resolve returning in 1s does not mean the process leaves in 1s. At the default--timeout 120neither is easy to hit in practice.New
-#test=dnstimeoutdrives a mock resolver that blocks 3s past a 1s--timeout; five of its six checks fail on master.