[PATCH 4 of 6] QUIC: never disable QUIC socket events

[Roman Arutyunyan]

Hi,

On Tue, Dec 13, 2022 at 08:57:12PM +0300, Maxim Dounin wrote:
> Hello!
> 
> On Fri, Dec 09, 2022 at 09:38:50AM +0000, Roman Arutyunyan wrote:
> 
> > # HG changeset patch
> > # User Roman Arutyunyan <arut at nginx.com>
> > # Date 1670256830 0
> > #      Mon Dec 05 16:13:50 2022 +0000
> > # Branch quic
> > # Node ID de8bcaea559d151f5945d0a2e06c61b56a26a52b
> > # Parent  b5c30f16ec8ba3ace2f58d77d294d9b355bf3267
> > QUIC: never disable QUIC socket events.
> > 
> > Unlike TCP accept(), current QUIC implementation does not require new file
> > descriptors for new clients.  Also, it does not work with accept mutex since
> > it normally requires reuseport option.
> > 
> > diff --git a/src/event/ngx_event_accept.c b/src/event/ngx_event_accept.c
> > --- a/src/event/ngx_event_accept.c
> > +++ b/src/event/ngx_event_accept.c
> > @@ -416,6 +416,12 @@ ngx_disable_accept_events(ngx_cycle_t *c
> >  
> >  #endif
> >  
> > +#if (NGX_QUIC)
> > +        if (ls[i].quic) {
> > +            continue;
> > +        }
> > +#endif
> > +
> 
> As long as the reuseport option is used, this should happen 
> automatically.  If it's not, it might be a bad idea to do not 
> disable accepting new QUIC connections, given that QUIC 
> connections can still use file descriptors for other tasks, such 
> as opening files and connecting to backends.
> 
> On the other hand, existing QUIC implementation cannot receive 
> packets from previously created connections if socket events are 
> disabled.  And this might be a good reason for the change.  
> If this is the reason, shouldn't it be in the commit log?

Exactly.  With current QUIC implementation, QUIC listen sockets aren't used
only for "listening", but also for receiving QUIC packets for existing
connections.

> Also, it looks like with client sockets, as introduced later in 
> this patch series, it would be appropriate to actually disable 
> events on QUIC listening sockets.

IMHO, even when client sockets are committed, the current approach will still
be available as an option.  Client sockets approach has a number of issues,
which the current implementation does not have.

> >          if (ngx_del_event(c->read, NGX_READ_EVENT, NGX_DISABLE_EVENT)
> >              == NGX_ERROR)
> >          {

Also, it looks like the problem is not specific to QUIC, but also exists for
regular Stream/UDP and should be addressed separately as a fix for mainline.
Probably we need a listen flag which would signal if the listen socket should
never be disabled, or just a more sophisticated condition in
ngx_disable_accept_events().  Anyway, I'll remove the patch from this series.

> > 
> > _______________________________________________
> > nginx-devel mailing list -- nginx-devel at nginx.org
> > To unsubscribe send an email to nginx-devel-leave at nginx.org
> 
> -- 
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan