Skip to content

Set a default request timeout - #16972

Merged
rullzer merged 2 commits into
masterfrom
enh/default_client_timeout
Sep 4, 2019
Merged

Set a default request timeout#16972
rullzer merged 2 commits into
masterfrom
enh/default_client_timeout

Conversation

@rullzer

Copy link
Copy Markdown
Member

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>

@juliusknorrjuliusknorr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, makes sense 👍

@rullzerrullzer 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
Copy Markdown
Contributor

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

@rullzer

Copy link
Copy Markdown
MemberAuthor

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

andrewperry commented Sep 2, 2019

Copy link
Copy Markdown

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
Copy Markdown
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>
@kesselb
kesselbforce-pushed the enh/default_client_timeout branch from fe439ea to 773778dCompareSeptember 3, 2019 12:59
@rullzer
rullzer merged commit b7301f4 into masterSep 4, 2019
@rullzer
rullzer deleted the enh/default_client_timeout branch September 4, 2019 06:08
@rullzerrullzer mentioned this pull request Sep 4, 2019
16 tasks
This was referenced Nov 3, 2019
@InfamousUser

InfamousUser commented Jan 24, 2020

Copy link
Copy Markdown

@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
Copy Markdown
Member

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

@rscircus

Copy link
Copy Markdown

Nice, thanks!

@InfamousUser

Copy link
Copy Markdown

Wonderful!

@kesselbkesselb mentioned this pull request Nov 28, 2022
4 tasks
@bernd-wechner

Copy link
Copy Markdown

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
Copy Markdown
Member

File a feature request please.

This is an old PR

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishenhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@rullzer@kesselb@andrewperry@nickvergessen@InfamousUser@rscircus@bernd-wechner@ChristophWurst@juliusknorr@TP75