Retry on Connection Errors

[kindly]

Request errors can happen intermittently for a variety of reasons i.e patchy internet, hyper holding on to request for too long, TLS errors, broken pipes.

This increases the types of issues that can be retried.

Which issue does this PR close?

Closes #4119

Rationale for this change

Only certain request errors are retried currently, and I was experiencing very unreliable multipart uploads, because if a single chunk failed the whole upload would fail. Mulitpart uploads caused more issues due to sometimes being a delay between the put requests for each chunk and hyper, sometimes using a dead connection, or having network issues.

What changes are included in this PR?

Allow retries when reqwest fails due to request errors.

Are there any user-facing changes?

Fewer failures when getting or uploading data to object stores.

[tustvold]

Unless I am mistaken this will retry request and connection timeouts, along with malformed responses from the server, is this desirable?

Perhaps we could do something like

if let Some(e) = e.source().downcast_ref::<hyper::Error>() {
    if e.is_connect() || e.is_closed() {
        // Do retry
    }
}

This would allow limiting the potential blast radius of this change?

[crepererum]

I agree that only server side (5xx) and transport errors should be retried, not all error codes. Esp. client side errors (4xx) should NOT be blindly retried.

So I guess the system could be: if it is an HTTP error code, filter the number range to 5xx. For all other errors use the flags to figure out their nature (is_connect/is_closed etc.).

[kindly]

@tustvold
I have basically done what you have said here. I added another condition is_incomplete_message() which was coming up for me when loading a large file, on an unstable connection.

[kindly]

@crepererum
These retries only cover cases where there is no response at all, therefore no response codes at all. The retry condition for requests with responses is is_server_error which I assume means 5xx errors.

[tustvold]

I think it would be good to get some test coverage of this change, perhaps by extending the existing MockServer based test, as I'm somewhat apprehensive it might lead to retrying errors that don't make sense to retry - e.g. timeouts, user-initiated aborts, etc...

[kindly]

@tustvold I have had difficulty making a useful test case for this. The MockServer currently works on the level of a Response object, and this will not have one, only a hyper error. I have not been able to find a way to make the mock server to misbehave on purpose, like prematurely close a connection. I think I would need to get the raw socket, but even with this I am not sure how realistic a test case I could make.

[tustvold]

I have not been able to find a way to make the mock server to misbehave on purpose, like prematurely close a connection

What happens if you MockServer::push_fn a function that panics, I think this should drop the connection without returning a response?

[kindly]

@tustvold yes, that did work.
It resulted in an incomplete message error on the client that is now caught and retried.

Added tests where the retry succeeded, and when it failed after max retries.

[tustvold]

Thank you