Repository navigation
fix: improve retries and timeout handling - #179
Conversation
cf7336f to
f1f76e4
Compare
719c0a4 to
c93dfe9
Compare
vprashar2929
changed the title
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Thanks!
Some suggestions, but overall nice!
| http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: | ||
| if retries < maxRetries { | ||
| response.Ctx.Put(numberOfRetriesKey, strconv.Itoa(retries+1)) | ||
| v.remoteLinks[originalURL] = fmt.Errorf("%q not accessible, status code %v, retrying: %w", response.Request.URL.String(), response.StatusCode, err) |
There was a problem hiding this comment.
Why we set it if it will be overridden later?
| // Install the transport once to avoid races between async requests. | ||
| v.c.WithTransport(promhttp.RoundTripperFunc(func(r *http.Request) (*http.Response, error) { | ||
| return promhttp.InstrumentRoundTripperCounter( | ||
| linktransformerMetrics.collyRequests, |
There was a problem hiding this comment.
Calling promhttp.InstrumentRoundTripperCounter and promhttp.InstrumentRoundTripperDuration inside RoundTripperFunc runs promhttp label validation and allocates a temporary prometheus.Registry on every request, redirect, and retry. Maybe wrap InstrumentRoundTripperCounter once outside RoundTripperFunc and observe collyPerDomainLatency directly?
| } | ||
| retryAfter := response.Headers.Get("Retry-After") | ||
| // Limit the parsed integer to avoid overflowing time.Duration when converting seconds. | ||
| if seconds, err := strconv.ParseUint(retryAfter, 10, 32); err == nil { |
There was a problem hiding this comment.
ParseUint(..., 10, 32) and http.ParseTime allow Retry-After values of hours or days, such as Retry-After: 3600, which will block mdox in OnError since v.c.Context has no deadline by default. Should we cap the maximum Retry-After delay or fail fast when a server asks to wait too long?
| var requests int32 | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| atomic.AddInt32(&requests, 1) | ||
| timer := time.NewTimer(31 * time.Second) |
There was a problem hiding this comment.
Since make test does not pass -short, this 31s sleep adds 31s to every test run. Could we test a short configured timeout like 100ms or check the configured transport field directly so unit tests stay fast?
There was a problem hiding this comment.
or use synctest
Not sure if synctest is available in the go version that we are using. But will take a look at it
|
@bwplotka About the Slack link CI failure since Slack returns an unsupported browser message with Colly’s default User-Agent, but the same request with a browser User-Agent returns 200 locally. Would you prefer adding an ignore rule for this specific URL, or changing the User-Agent that mdox sends? WDYT? |
|
I would ask if it makes to query slack urls on CI. On most or all cases you need to be authenticated. I would ensure users can put slack on some ignore list of known urls. I would reduce unnecessary network to endpoint to be a good citizen. |
Link checks can fail when a site drops a connection or returns a temporary error. Retry these requests up to three times with backoff, respect Retry-After, and stop retries when the context is canceled. Signed-off-by: vprashar2929 <vibhu.sharma2929@gmail.com>
Link checks can fail when a site drops a connection or returns a
temporary error. Retry these requests up to three times with backoff,
respect Retry-After, and stop retries when the context is canceled.