Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time restrict network operations #3157

Merged
merged 5 commits into from
Oct 20, 2023
Merged

Conversation

infeo
Copy link
Member

@infeo infeo commented Oct 17, 2023

Closes #3113

This PR adds to all network requests the timeout parameter, in order to detect and handle "no connection possible" events. It adds the time out to the request itself. For the authentication, this is not possible over the API, hence, only a connect timeout is set.

The timeout varies between 3 and 5 seconds.

@infeo infeo added this to the 1.11.0 milestone Oct 17, 2023
Copy link
Member

@SailReal SailReal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend increasing the timeout to at least 10 seconds. If we think of a bad hotel or "Deutsche Bahn" WiFi, the user is connected to the corporate VPN and then wants to perform an unlock on a stressed hub server, giving up after 3 seconds is way too early in my opinion.

The default of OkHTTP is 5s for connect, read, write and 10s for the hole call (including DNS etc):

@tobihagemann tobihagemann removed their request for review October 19, 2023 16:55
@overheadhunter overheadhunter added the type:enhancement Improvements on an existing feature label Oct 20, 2023
Copy link
Member

@SailReal SailReal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM but because of

This does not add a timeout to the timespan between start of the Auth Flow and its fulfillment (i.e. redirecting back to the app).

(from coffeelibs/tiny-oauth2-client#5)

this does not fully solve the motivation of #3113

This can confuse users due to missing feedback/endless response waiting, especially, if different applications with explicit proxy settings have internet access.

because it is still true that if the server takes infinitely long to respond (e.g. no internet connection) Cryptomator waits forever for the response while trying to unlock a Hub vault.

So I think we should add a timeout waiting for the redirect as well, but if you prefer we can do that in another PR later.

@infeo infeo merged commit 800b244 into develop Oct 20, 2023
7 checks passed
@infeo infeo deleted the feature/3113-network-timeout branch October 20, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Improvements on an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add timeout to network requests
3 participants