Hello!

On Mon, Jun 29, 2015 at 03:55:25PM -0700, Andy Isaacson wrote:

> # HG changeset patch
> # User Andy Isaacson <adi@orionlabs.co>
> # Date 1435618451 25200
> # Mon Jun 29 15:54:11 2015 -0700
> # Node ID c11304760218324ea55de7250a613af8f13e431b
> # Parent b95e70ae6bcdbae99a967df01e1011839f19ee0e
> Add support for tcp_user_timeout in http listen directive
>
> This commit adds support for a new tcp_user_timeout=<timeout_sec>
> parameter to the listen directive. When enabled and set to a value
> greater than zero, the TCP_USER_TIMEOUT sockopt is set. From tcp(7):
>
> This specifies the maximum amount of time in milliseconds
> that transmitted data may remain unacknowledged before TCP
> will forcibly close the corresponding connection and return
> ETIMEDOUT to the application.
>
> Without this capability, a HTTP longpoll connection can remain active
> for up to 950 seconds after the last ACK from the client.
>
> Note that the tcp_user_timeout value is specified in (integer) seconds,
> but the setsockopt API is specified in milliseconds.
>
> This capability is similar to the systemwide configuration
> net.ipv4.tcp_retries2 on Linux, but more flexible and per-socket.

It would be a good idea to clarify expected use cases and how it's
different from SO_KEEPALIVE / TCP_KEEPCNT / TCP_KEEPIDLE /
TCP_KEEPINTVL we already have.

>
> diff -r b95e70ae6bcd -r c11304760218 auto/unix
> --- a/auto/unix Thu Sep 05 16:53:02 2013 +0400
> +++ b/auto/unix Mon Jun 29 15:54:11 2015 -0700
> @@ -330,6 +330,18 @@
> . auto/feature
>
>
> +ngx_feature="TCP_USER_TIMEOUT"
> +ngx_feature_name="NGX_HAVE_TCP_USER_TIMEOUT"
> +ngx_feature_run=no
> +ngx_feature_incs="#include <sys/socket.h>
> + #include <netinet/in.h>
> + #include <netinet/tcp.h>"
> +ngx_feature_path=
> +ngx_feature_libs=
> +ngx_feature_test="setsockopt(0, IPPROTO_TCP, TCP_USER_TIMEOUT, NULL, 0)"
> +. auto/feature
> +
> +

Probably putting the test after the TCP_KEEPIDLE test would be
more logical.

> ngx_feature="TCP_KEEPIDLE"
> ngx_feature_name="NGX_HAVE_KEEPALIVE_TUNABLE"
> ngx_feature_run=no
> diff -r b95e70ae6bcd -r c11304760218 src/core/ngx_connection.h
> --- a/src/core/ngx_connection.h Thu Sep 05 16:53:02 2013 +0400
> +++ b/src/core/ngx_connection.h Mon Jun 29 15:54:11 2015 -0700
> @@ -80,6 +80,10 @@
> int setfib;
> #endif
>
> +#if (NGX_HAVE_TCP_USER_TIMEOUT)
> + int tcp_user_timeout;
> +#endif
> +
> };
>
>
> diff -r b95e70ae6bcd -r c11304760218 src/event/ngx_event_accept.c
> --- a/src/event/ngx_event_accept.c Thu Sep 05 16:53:02 2013 +0400
> +++ b/src/event/ngx_event_accept.c Mon Jun 29 15:54:11 2015 -0700
> @@ -284,6 +284,23 @@
> }
> }
>
> +#if (NGX_HAVE_TCP_USER_TIMEOUT)
> +#ifdef TCP_USER_TIMEOUT
> + if (ls->tcp_user_timeout) {
> + int value = ls->tcp_user_timeout;
> +
> + if (setsockopt(s, IPPROTO_TCP, TCP_USER_TIMEOUT, &value,
> + sizeof(int))
> + == -1)
> + {
> + ngx_log_error(NGX_LOG_ALERT, log, ngx_socket_errno,
> + "setsockopt(TCP_USER_TIMEOUT, %d) for %V failed",
> + value, c->addr_text);
> + }
> + }
> +#endif
> +#endif
> +
> #if (NGX_DEBUG)
> {
>

This looks like a very wrong file to put the code. The option
should be set on a listening socket instead. Doing an extra
syscall on each accept() is clearly bad idea.

There is no need to check if the TCP_USER_TIMEOUT macro is defined
twice. It's already tested by the configure test, and there is no
need to test it again.

Please add

[diff]
showfunc = true

to your ~/.hgrc to simplify review.

> diff -r b95e70ae6bcd -r c11304760218 src/http/ngx_http.c
> --- a/src/http/ngx_http.c Thu Sep 05 16:53:02 2013 +0400
> +++ b/src/http/ngx_http.c Mon Jun 29 15:54:11 2015 -0700
> @@ -1800,6 +1800,10 @@
> ls->deferred_accept = addr->opt.deferred_accept;
> #endif
>
> +#if (NGX_HAVE_TCP_USER_TIMEOUT && defined TCP_USER_TIMEOUT)
> + ls->tcp_user_timeout = addr->opt.tcp_user_timeout;
> +#endif
> +
> #if (NGX_HAVE_INET6 && defined IPV6_V6ONLY)
> ls->ipv6only = addr->opt.ipv6only;
> #endif
> diff -r b95e70ae6bcd -r c11304760218 src/http/ngx_http_core_module.c
> --- a/src/http/ngx_http_core_module.c Thu Sep 05 16:53:02 2013 +0400
> +++ b/src/http/ngx_http_core_module.c Mon Jun 29 15:54:11 2015 -0700
> @@ -4085,6 +4085,31 @@
> continue;
> }
>
> + if (ngx_strncmp(value[n].data, "tcp_user_timeout=", 17) == 0) {
> +#if (NGX_HAVE_TCP_USER_TIMEOUT && defined TCP_USER_TIMEOUT)
> + int timeout_sec = ngx_atoi(value[n].data + 17, value[n].len - 17);
> +
> + /*
> + * convert from seconds (in config file) to milliseconds (for
> + * setsockopt)
> + */
> + lsopt.tcp_user_timeout = timeout_sec * 1000;

Consider using ngx_parse_time() instead.

> +
> + if (lsopt.tcp_user_timeout < 0) {
> + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> + "Invalid tcp_user_timeout \"%V\"",

The message doesn't match style of other messages.

> + &value[n]);
> + return NGX_CONF_ERROR;
> + }
> +#else
> + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> + "tcp_user_timeout \"%V\" is not supported on "

The "%V" is useless here.

> + "this platform, ignored",
> + &value[n]);
> +#endif
> + continue;
> + }
> +
> if (ngx_strcmp(value[n].data, "deferred") == 0) {
> #if (NGX_HAVE_DEFERRED_ACCEPT && defined TCP_DEFER_ACCEPT)
> lsopt.deferred_accept = 1;
> diff -r b95e70ae6bcd -r c11304760218 src/http/ngx_http_core_module.h
> --- a/src/http/ngx_http_core_module.h Thu Sep 05 16:53:02 2013 +0400
> +++ b/src/http/ngx_http_core_module.h Mon Jun 29 15:54:11 2015 -0700
> @@ -102,6 +102,10 @@
> ngx_uint_t deferred_accept;
> #endif
>
> +#if (NGX_HAVE_TCP_USER_TIMEOUT && defined TCP_USER_TIMEOUT)
> + int tcp_user_timeout;
> +#endif
> +

The field ordering is type-based here.
Consider putting this somewhere after tcp_keepidle instead.

> u_char addr[NGX_SOCKADDR_STRLEN + 1];
> } ngx_http_listen_opt_t;
>
>

--
Maxim Dounin
http://nginx.org/

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel