[PATCH] Resolver: make TCP write timer event cancelable.

[Maxim Dounin]

Hello!

On Thu, May 26, 2022 at 03:50:22PM +0000, Aleksei Bavshin via nginx-devel wrote:

> # HG changeset patch
> # User Aleksei Bavshin <a.bavshin at f5.com>
> # Date 1653579686 25200
> #      Thu May 26 08:41:26 2022 -0700
> # Branch se

Looks like a wrong branch to me.

> # Node ID afb3646a08b6ebcf46fdbd2e7bf97cdcccca774b
> # Parent  5a98e9cb437f7719afa2bde62de68e174fd8e03e
> Resolver: make TCP write timer event cancelable.
> 
> In certain circumstances, this timer could become the only remaining
> non-cancelable timer in a worker, effectively delaying the worker shutdown.
> 
> We're keeping the resolver alive to avoid disrupting any leftover tasks in the
> worker that may require up-to-date upstream peer data, but it is unnecessary
> in absence of other connections. And any useful workload would have its own
> non-cancelable timers, so it is safe to allow canceling this one.

This doesn't sound correct to me: when nginx is doing upstream 
dynamic name-to-address resolution during request processing (see 
ngx_http_upstream_init_request()), the request has no "own 
non-cancelable timers".

Still, there should be resolve task timer, and canceling the tcp 
timer should be safe, similarly to canceling the resolver resend 
timer, see 70e65bf8dfd7.  And, BTW, it might be a good idea to 
explicitly mention this changeset in the commit log.

> 
> The scenario in question is a periodic resolver task with a period less than
> the TCP write timeout and responses large enough to warrant fallback to a TCP
> connection - something that could be reproduced with a dynamic upstream
> implementation. With each event loop wakeup, we either see a previously
> set write timer instance or schedule a new one.
> 
> diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
> --- a/src/core/ngx_resolver.c
> +++ b/src/core/ngx_resolver.c
> @@ -1624,6 +1624,7 @@ ngx_resolver_send_tcp_query(ngx_resolver
>          }
>  
>          rec->tcp->data = rec;
> +        rec->tcp->write->cancelable = 1;
>          rec->tcp->write->handler = ngx_resolver_tcp_write;
>          rec->tcp->read->handler = ngx_resolver_tcp_read;
>          rec->tcp->read->resolver = 1;

>From style point of view, it would be a good idea to set flags 
after the handler.

-- 
Maxim Dounin
http://mdounin.ru/