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

[Aleksei Bavshin]

> -----Original Message-----
> From: Maxim Dounin <mdounin at mdounin.ru>
> Sent: Thursday, May 26, 2022 3:31 PM
> To: Aleksei Bavshin via nginx-devel <nginx-devel at nginx.org>
> Subject: Re: [PATCH] Resolver: make TCP write timer event cancelable.
> 
> EXTERNAL MAIL: nginx-devel-bounces at nginx.org
> 
> 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.

Yep, I was already informed about that. As the main reproducer for the problem is the upstream code from 'se', I had to use this branch for verification and forgot to rebase. Sorry ☹
The change is in the oss portion of the code though and applies cleanly. I'll make sure to rebase any further patches on the right branch.

> 
> > # 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".

Sorry, I've likely used a wrong terminology here.

An upstream server with 'resolve' parameter monitors the address changes independently from http request processing.  There's an effort made to keep these resolver tasks cancelable (including 70e65bf8dfd7 you mentioned), while ensuring that the resolver connections are closed last - ngx_shutdown_timer_handler() ignores connections with resolver flag.
As I understand the intention, we wanted to keep updating the upstream servers until the last http (stream, mail, etc) connection is closed and only then allow the worker to shut down. The non-cancelable timer in resolver interferes with this last part.

I'll rewrite the description to make that clearer.

> 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.

Ok, will do in v2.

> 
> --
> Maxim Dounin
> https://nam02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmdo
> unin.ru%2F&data=05%7C01%7CA.Bavshin%40F5.com%7C3c7d773d6815
> 4de9e63008da3f6790cd%7Cdd3dfd2f6a3b40d19be0bf8327d81c50%7C0%7C0
> %7C637892011303441223%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=3G6yY5Ryx6a%2BXsQowcL0V%2F%2F217ByGKWKHAiVgalH
> up0%3D&reserved=0
> _______________________________________________
> nginx-devel mailing list -- nginx-devel at nginx.org
> To unsubscribe send an email to nginx-devel-leave at nginx.org