search

ramsayleung / rspotify

Spotify Web API SDK implemented on Rust
MIT License
639 stars 123 forks source link

enable / standardize client request timeouts #394

Closed eladyn closed 1 year ago

eladyn commented 1 year ago

Description

This sets the same timeout duration for reqwest and ureq http clients.

Motivation and Context

https://github.com/ramsayleung/rspotify/blob/6d304de632a84f79c35cb9ff4a0e66be238cf205/rspotify-model/src/auth.rs#L79-L80

This mentions a maximum request time of 10 seconds, but AFAICT, these timeouts aren't specified anywhere. By default, reqwest does not have a timeout at all, ureq has a 30 second connect timeout.

Type of change

Please delete options that are not relevant.

How has this been tested?

The provided non-modifying tests.

Is this change properly documented?

I hope so. :) Please point me to additional documentation changes that are necessary, if there are any.

ramsayleung commented 1 year ago

This mentions a maximum request time of 10 seconds, but AFAICT, these timeouts aren't specified anywhere. By default, reqwest does not have a timeout at all, ureq has a 30 second connect timeout.

Thanks for your fix, you make a great point. These two HTTP clients do has a different default timeout setting.

But I want to clarify the code you are referring, I am not sure whether you are misunderstanding the comment.

https://github.com/ramsayleung/rspotify/blob/6d304de632a84f79c35cb9ff4a0e66be238cf205/rspotify-model/src/auth.rs#L79-L80

The 10 seconds margin in the comment, means the margin of token expiration, instead of the HTTP connect timeout.

For example, if the expiration time is 2023-03-10 00:00:10, the auth flow will check and assert that it's expired if the current time is later than 2023-03-10 00:00:00, the 10 seconds is a margin.

From this point, it makes sense to set the HTTP connect timeout to 10 seconds :)

eladyn commented 1 year ago

The 10 seconds margin in the comment, means the margin of token expiration, instead of the HTTP connect timeout.

Yeah, that makes sense.

From this point, it makes sense to set the HTTP connect timeout to 10 seconds :)

Do you mean that rather than applying a general timeout (which includes the connect, write and read phase), we should only be setting a connect timeout? Or is it fine the way I did it?

ramsayleung commented 1 year ago

Or is it fine the way I did it?

Your approach is great. However, I would like to clarify any potential misunderstandings :)