-
Notifications
You must be signed in to change notification settings - Fork 24
Timeouts #185
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
Timeouts #185
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've raised a pull request (#188) against yours that moves all the configuration into nginx.conf
so its only managed in a single place. In reviewing your changes I've noticed that the reverse proxy for tomcat is no longer needed. Could you please review the other outstanding pull requests that are this repo?
nginx/Dockerfile
Outdated
NGINX_KEEPALIVE_TIMEOUT=65 \ | ||
NGINX_LINGERING_TIMEOUT=60 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is actual 5 seconds: https://nginx.org/en/docs/http/ngx_http_core_module.html#lingering_timeout
@@ -53,6 +62,8 @@ ENV \ | |||
PHP_MAX_INPUT_VARS=3000 \ | |||
PHP_MEMORY_LIMIT=256M \ | |||
PHP_POST_MAX_SIZE=128M \ | |||
PHP_PROCESS_CONTROL_TIMEOUT=60 \ | |||
PHP_REQUEST_TERMINATE_TIMEOUT=60 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is 0
according to the documentation above for both of the php
settings?
nginx/Dockerfile
Outdated
NGINX_CLIENT_MAX_BODY_SIZE=0 \ | ||
NGINX_ERROR_LOG_LEVEL=warn \ | ||
NGINX_FASTCGI_READ_TIMEOUT=60 \ | ||
NGINX_FASTCGI_SEND_TIMEOUT=60 \ | ||
NGINX_FASTCGI_CONNECT_TIMEOUT=60 \ | ||
NGINX_KEEPALIVE_TIMEOUT=65 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this isn't the same as the default: https://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_timeout
tomcat/Dockerfile
Outdated
NGINX_PROXY_CONNECT_TIMEOUT=60 \ | ||
NGINX_SEND_TIMEOUT=60 \ | ||
NGINX_WORKER_CONNECTIONS=1024 \ | ||
NGINX_WORKER_PROCESSES=auto \ | ||
TOMCAT_ADMIN_NAME=admin \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NGINX_WORKER_CONNECTIONS
and NGINX_WORKER_PROCESSES
are not used.
@@ -58,6 +58,6 @@ RUN --mount=type=cache,id=demo-composer,sharing=locked,target=/root/.composer/ca | |||
mkdir -p /var/www/drupal/web/sites/default/files/library-definitions && \ | |||
cp /var/www/drupal/web/modules/contrib/openseadragon/openseadragon.json /var/www/drupal/web/sites/default/files/library-definitions | |||
|
|||
FROM ${repository}/drupal:${tag} | |||
FROM ${repository}/drupal:${tag} AS drupal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do anything as far as I can tell, not sure why it's included?
…ywhere as defaults (#188) Co-authored-by: Nigel Banks <[email protected]:w>
Oh nice, thanks @nigelgbanks ! I have a follow up PR for this with an extra tomcat timeout I missed. Also big thanks for getting rid of the extra reverse proxy layer! |
Here is every possible timeout you might wanna set if you're running into weird errors with really really big files. This won't change anything for existing installs, as all defaults are still the same. But this way you can add them to your docker-compose.yml and play with them if you need it.
At born digital we've used this to get a couple of clients out of a bind with ingests of very large files.