Connecting to homeserver without installed certificates causes bootloop

[manuel2258]

Hi, I'm currently working on a small university project using this SDK and I came across a problem without a proper error message.

Describe the bug
Trying to connect to a homeserver without having any certificates installed (bare-bones docker container) causes the Client::new function to enter a infinite loop which does not produce a proper error message.

To Reproduce
I currently don't have to much time, but I would like to add a sample project in the future if needed.
Steps to reproduce the behavior:

1. Bundle binary without installed certificates in docker container
2. Start binary

Expected behavior
Connection fails with clear error message.

Logs
logs.txt

Desktop:

• Distributor ID: Ubuntu
• Description: Ubuntu 20.04.3 LTS
• Release: 20.04
• Codename: focal

Rust:

• cargo 1.57.0 (b2e52d7ca 2021-10-21)
• rustc 1.57.0 (f1edd0429 2021-11-29)
• matrix-sdk 0.4.1
• reqwest 0.11.9
• hyper 0.14.16

Additional context
As I already mentioned I'm busy this week, but would love to provide a proper test project next week if needed. Also I would be interested in fixing the issue myself, if possible.

[poljar]

Client::new() doesn't send out any requests, did you mean Client::new_from_user_id()?

[manuel2258]

oops, I mean Client::login.
Creating using Client::new works fine, and then trying to log in using Client::login causes the loop.

[poljar]

Ah ok. The client isn't really stuck in an infinite loop but it does retry to send the request for way too long. I remember someone fixing this but I guess it never got merged.

The affecting line is here:

matrix-rust-sdk/crates/matrix-sdk/src/client.rs

Line 944 in 8538bdc

let response = self.send(request, None).await?;

The None parameter there is a Option<RequestConfig> where we can modify the retry logic to tailor the needs of such an request.

We still probably want to retry this request at least a couple of times, 2-3 times sounds fine. Wei might want to create a new constructor for RequestConfig that will create a short_retry() configuration.

[manuel2258]

Alright, sounds good!

May I tackle this issue or will you guys do?

[poljar]

Feel free to tackle it.

[manuel2258]

Alright, I did some digging and yes the solution is of course just as u suggested to provide a RequestConfig with a retry_limit set.

However from just glancing at it it seems weird that the retry limit is set to unlimited in RequestConfig::default. Wouldn't it make more sens to limit it by default, otherwise the same error will not be caught when the user does not use the simple Client::login method.

Or is there a good reason against it?

[poljar]

It's just what we ended up with as a first cut of the retry logic, I don't think it's unlimited it should be what the backoff crate has as default.

As mentioned, some constructors for RequestConfig should be introduced and possibly the Default implementation should switch to RequestConfig::short_retry() or some medium value.

[manuel2258]

Alright, I have a few more questions:

1. I suppose changing the Default to RequestConfig::short_retry() might have to many side effects, therefor I would just apply it to Client::login. Should we then also add it to Client::login_with_token?
2. client::test::no_retry_http_requests is of course now failing, which points out another problem. It seemed like that others connection issues would result in login to fail on the first try, like a 501 response from the homeserver. Having RequestConfig::short_retry() would also mean that such connections are retried. Is that something that we want / would be okey?

[poljar]

Alright, I have a few more questions:

1. I suppose changing the `Default` to `RequestConfig::short_retry()` might have to many side effects, therefor I would just apply it to `Client::login`. Should we then also add it to `Client::login_with_token`?

Yes, I think all the login methods, the registry method and the version check method should use a short retry. Later on we probably should have a medium retry for the majority of request types and a long retry for the sync loop.

2. `client::test::no_retry_http_requests` is of course now failing, which points out another problem. It seemed like that others connection issues would result in login to fail on the first try, like a 501 response from the homeserver. Having `RequestConfig::short_retry()` would also mean that such connections are retried. Is that something that we want / would be okey?

I think the family of 500 errors should be retried as well.

[manuel2258]

I provided a PR, looking forward to feedback.

I suppose with registry and version check you meant Client::register and Client::get_supported_versions.

[poljar]

Closed by #506.