Repository navigation
fix(flaresolverr): tell a slow server apart from an absent one - #143
Merged
Merged
Conversation
Three faults found reviewing the package after it merged, all in how a failure is reported rather than in what it does. Every transport error was reported as "cannot reach ... is it running, and is the url right?", including timeouts. So FlareSolverr chewing on a hard challenge past the deadline told the user to go and check a server they could see running, when the fix is more time. A daemon shutting down mid-search said the same thing about nothing being wrong at all. This is the distinction the Error type exists for, and internal/indexer already draws it; the two packages now classify alike, by asking the error rather than consulting a clock. The status was captured and never rendered, so a failure FlareSolverr would not explain printed its URL and then nothing. Reachable by pointing the url at something that answers JSON but is not FlareSolverr - a reverse proxy's error page, another service on that port. The status is the only fact left in that case, and it is now what gets said. Errors also distinguish "FlareSolverr failed" from "that was not FlareSolverr", which are different things to go and look at. The per-request bound lived only in the http.Client built when none was supplied, so a caller passing its own - to share a transport, as the plugin is likely to - silently lost it: a server that accepts the connection and never answers would hang the call forever. The deadline moves into send, where it holds whichever client is in use. Each of the three has a test that fails without its fix, checked by reverting them: the timeout one reports "cannot reach ... is it running", the bound one hangs out its ten second limit, and the no-message one prints a URL followed by nothing. Those tests also needed the wait to be short, which it could not be: the bound is the timeout plus a fixed fifteen second slack. Config grows an unexported slack, the same trick internal/indexer uses for its clock. And the stand-in server waits on a channel the test closes rather than on the request context, because a handler that never returns makes httptest's Close hang - a test that fails by hanging is a bad test.
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
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.
Three faults found reviewing #142 after it merged, all in how a failure is
reported rather than in what the package does.
Every transport error was reported as unreachable, including timeouts:
So FlareSolverr chewing on a hard challenge past the deadline told you to check a
server you could watch running, when the fix is more time. A daemon shutting down
mid-search said the same about nothing being wrong at all. This is the
distinction
Errorexists for, andinternal/indexeralready draws it - the twopackages now classify alike, by asking the error rather than consulting a clock.
The status was captured and never rendered, so a failure FlareSolverr would
not explain printed its URL and then nothing. Reachable by pointing the url at
something that answers JSON but is not FlareSolverr - a reverse proxy's error
page, another service on that port. The status is the only fact left in that
case, and it is now what gets said.
KindServerandKindBadReplyalso separate"FlareSolverr failed" from "that was not FlareSolverr", which are different
things to go and look at.
The per-request bound lived only in the default
http.Client, so a callersupplying its own - to share a transport, as the rutracker plugin is likely to -
silently lost it. A server that accepts the connection and never answers would
hang the call forever. The deadline moves into
send, where it holds whicheverclient is in use.
Each fix has a test that fails without it
Verified by reverting each one:
kind = 0, want KindTimeout+ the "is it running" messagesendflaresolverr: http://127.0.0.1:50159:— nothing after the colonTwo things the tests themselves needed
The bound is
timeout + 15s, so a test could not make it short:Configgrowsan unexported
slack, the same trickinternal/indexeruses for its clock.And the stand-in server waits on a channel the test closes rather than on the
request context - a handler that never returns makes
httptest'sClosehang,and a test that fails by hanging is a bad test. I know because mine did, for ten
minutes, before I fixed it.
make checkandgo test -raceclean; the suite runs in 0.25s.