Add retry support to ruma-client

[jplatte]

The shortcoming you see

Currently, ruma-client never retries failed requests. Implementing retries on top of ruma-client means re-serializing requests, which is not ideal.

The improvement

Make it easier to do request retries w/o serializing the same request again, in one of these ways:

1. Direct support
   • pro: Makes it easy to use ruma-client in a more robust way
   • con: Larger API surface
2. Callback API
   • pro: Smaller API surface
   • con: Kind of ugly API?
3. Enhancing ruma-client's Error type (making the serialized request accessible from it)
   • pro: Maximally flexible
   • con: Also somewhat ugly?

I'm currently leaning towards option 3, but I'm not really certain.

edit: Option 3 also allows adding option 1 on top later.

[ShadowJonathan]

thought: Option 3 could enforce invariants of retryable and non-retryable responses

[ShadowJonathan]

Further thoughts; since matrix has rate limit responses (http response 429), I think adding that into the returned error and corresponding info would allow the application to call on their own "wait" function, as currently it's not really possible to do this agnostically, as every own async ecosystem has its own sleep function, which aren't interoperable.

So I'm leaning more and more towards option 3

[jplatte]

I will work on this (opt 3), I think it should actually be very simple.

[jplatte]

Seems unlikely I'll actually get to this anytime soon.

[avdb13]

I think it should actually be very simple

How so? While working on SSO support for Conduit today I stumbled upon the need for retryable requests without cloning the request after failure. Here is the code:

    pub async fn create_something(...) -> Result<Self> {
        for attempt in 1..=attempts {
            match make_some_fallible_async_request.await {
                Ok(metadata) => return Ok(Something::new(Metadata::from(metadata))),
                Err(e) => {},
            }
        }

        return Err(Error::bad_config(
            "Failed HTTP request.",
        ));
    }

[avdb13]

Guess Raw<T> fits this problem, just wondering how this would affect UiaaResponse since this is also a type of request we should retry.

[avdb13]

This seems like it's definitely gonna require cloning around a bit, and from what I noticed it seems like hyper needs to have the Request collected into a single chunk first. A bit awkward indeed.

[avdb13]

@jplatte where should cloning serialized requests happen ideally and how should retrying behave?

[jplatte]

So I just looked into this. Currently, ruma-client sits in a little bit of an uncomfortable place w.r.t. serialized request bodies when it comes to cloning, because the request body is an associated type on HttpClient that implements BufMut, which is not the thing we should be cloning (rather, that should be a Buf-implementing type which may or may not be the same underlying type).

I think the most reasonable fix for now is to remove this level of genericity from ruma-client since the two backends we have both use BytesMut / Bytes to represent serialized bodies anyawys.

That is, I'm thinking as a preparation PR:

• the HttpClient trait should have its RequestBody and ResponseBody associated types replaced by BytesMut and Bytes respectively
• the HttpClient::send_http_request method should change to use Bytes instead of BytesMut as the body, moving the freeze operation outside that trait method into callers

If this works out we should have a Request<Bytes> before all calls to HttpClient::send_http_request, a type that is cheap enough to clone (because after cloning, both request objects will reference the same body allocation). We just need to unconditionally copy it before calling send_http_request, and can "give out" the clone as part of the Result::Err if things go wrong. That request can then be re-sent as part of any retry flow people want to implement.