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.

Sign up for GitHub

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

Jump to bottom

nixos/nginx: set "recommended" proxy timeouts to 60s #118443

Merged
merged 1 commit into from Apr 4, 2021
Merged

nixos/nginx: set "recommended" proxy timeouts to 60s #118443

merged 1 commit into from Apr 4, 2021

Conversation

SuperSandro2000
Copy link
Member

According to the nginx documentation [1] those values cannot usually exceed 75 seconds.
The defaults are 60s and should probably be lowered to something reasonable like 20 or 30 seconds.

[1] https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@SuperSandro2000
nixos/nginx: set "recommended" proxy timeouts to 60s
db5a156 
According to the nginx documentation [1] those values  cannot usually exceed 75 seconds.
The defaults are 60s and should probably be lowered to something reasonable like 20 or 30 seconds.

[1] https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout
aanderse
aanderse approved these changes Apr 4, 2021
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Yeah 90 seconds is crazy high. Even 60 is a bit high... but maybe that is an opinionated statement based on some terrible personal experiences 😄

LGTM 👍

mweinelt
mweinelt approved these changes Apr 4, 2021
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Totally fine with going with the upstreams default value.

@mweinelt
Copy link
Member

mweinelt commented Apr 4, 2021

@GrahamcOfBorg eval

@mweinelt mweinelt merged commit c26f97a into NixOS:master Apr 4, 2021
@SuperSandro2000 SuperSandro2000 deleted the SuperSandro2000-patch-2 branch April 5, 2021 03:23
@SuperSandro2000
Copy link
Member Author

Yeah 90 seconds is crazy high. Even 60 is a bit high... but maybe that is an opinionated statement based on some terrible personal experiences 😄

LGTM 👍

Agreed. Under normal circumstances those should be 5 to 10 seconds and upstream_keep_alive can be 90 seconds.
Thats why I created #118521 to easily change them.

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

@mweinelt mweinelt mweinelt approved these changes

@aanderse aanderse aanderse approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SuperSandro2000 @mweinelt @aanderse