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

Set a default request timeout #16972

Merged
merged 2 commits into from
Sep 4, 2019
Merged

Set a default request timeout #16972

merged 2 commits into from
Sep 4, 2019

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Sep 2, 2019

This to avoid endless running processes.
A default timeout of 30 seconds should cover the 99% case. If a job need
specific longer time it should set that.

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

This to avoid endless running processes.
A default timeout of 30 seconds should cover the 99% case. If a job need
specific longer time it should set that.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Yes, makes sense 👍

@rullzer rullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 2, 2019
@kesselb
Copy link
Contributor

kesselb commented Sep 2, 2019

Please have a look at #16149 and why 30 seconds might be a bad default value.

@rullzer
Copy link
Member Author

rullzer commented Sep 2, 2019

Please have a look at #16149 and why 30 seconds might be a bad default value.

@kesselb that PR was about raising it to 30s.
But I'm fine with a followup for a different value. But for now the value is unlimited. And 30s seems more sane than unlimited 😉

@andrewperry
Copy link

andrewperry commented Sep 2, 2019

Here is where @kesselb suggested 30s might be sane (or at least more sane than 300s): #14926 (comment)

This seems to be affecting a lot of people at the moment, so I also wonder whether the https://apps.nextcloud.com is running slower at present. When I test that page with Google it is suggesting times upwards of 50 seconds for "Time to interactive": https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Fapps.nextcloud.com%2Fapi%2Fv1%2Fapps.json

This might mean that 30s won't fix the issue people are experiencing and that an initial DevOps focus within Nextcloud on increasing the speed of the response for https://apps.nextcloud.com/api/v1/apps.json would be good, without waiting for this patch to roll through.

@nickvergessen
Copy link
Member

Well the idea of this PR is not to solely fix the issue you mentioned, but fix a general misbehave with long running tasks.

So yeah the apps.json should still be looked into.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@rullzer rullzer merged commit b7301f4 into master Sep 4, 2019
@rullzer rullzer deleted the enh/default_client_timeout branch September 4, 2019 06:08
@rullzer rullzer mentioned this pull request Sep 4, 2019
16 tasks
This was referenced Nov 3, 2019
@InfamousUser
Copy link

InfamousUser commented Jan 24, 2020

@rscircus Not a crappy network thing. You can't reasonably expect a 300MB app to install in 30 seconds, not everyone runs Nextcloud on enterprise hardware. My connection allows it to download in about a minute in a pure, uncongested situation, not to mention the time it takes to actually install.

@nickvergessen
Copy link
Member

The app download has been adjusted to 120s in the upcoming 18.0.1 already

@rscircus
Copy link

Nice, thanks!

@InfamousUser
Copy link

Wonderful!

@kesselb kesselb mentioned this pull request Nov 28, 2022
4 tasks
@bernd-wechner
Copy link

This should be a setting available in config.php! 30 seconds did not suffice for us, and the consequences are a pain to diagnose (simply a lacking app store) and this and related issues a challenge to find. Clearly if performance is so poor on some networks it's no drama to up this to 300 (and then it does work) but one needs to know to do so, and perforce this setting belongs in config.php IMHO.

@ChristophWurst
Copy link
Member

File a feature request please.

This is an old PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants