[PATCH 2 of 3] HTTP/3: eliminated v3_session field from ngx_http_connection_t

[Roman Arutyunyan]

Hi,

On Tue, Sep 12, 2023 at 05:05:02PM +0400, Sergey Kandaurov wrote:
> 
> > On 11 Sep 2023, at 15:30, Roman Arutyunyan <arut at nginx.com> wrote:
> > 
> > # HG changeset patch
> > # User Roman Arutyunyan <arut at nginx.com>
> > # Date 1694431436 -14400
> > #      Mon Sep 11 15:23:56 2023 +0400
> > # Node ID 9ee4158b9d3fa41e647b772e707c29b3e4cb77b5
> > # Parent  65574b876c4047495ac8622c6161fc87c1b75913
> > HTTP/3: eliminated v3_session field from ngx_http_connection_t.
> > 
> > The field was under NGX_HTTP_V3 macro, which was a source of binary
> > compatibility problems when nginx/module is build with/without HTTP/3 support.
> > 
> > Now ngx_http_v3_session_t is assigned to c->data on streams initialization,
> > while ngx_http_connection_t object is referenced by http_connection field of
> > the session object, similar to ngx_http_v2_connection_t and ngx_http_request_t.
> 
> I'd write explicitly (taking from the 1st patch commit log) that session
> creation is now postponed, to make the description self contained, YMMV.
> 
> Note that this change will trigger segfaults on graceful shutdown
> if there are connections with handshake not yet complete,
> due to now postponed session creation.
> In this case, http3 shutdown() callback is called, used to send
> GOAWAY and proceed with connection close, but h3c is not yet.
> Probably calling the callback should depend on c->ssl->handshaked,
> because it has little sense if handshake isn't complete.

Thanks for noticing this.  Indeed, hc is treated as h3c, which may lead to a
crash.  The solution is to postpone calling shutdown() until after init().
I will add a separate patch for this.  An alternative solution could be
to terminate the handshake since the worker is exiting anyway.  However, as we
try to be as graceful as possible while shutting down the old worker, it's
better to finish the handshake and send GOAWAY after that.

> > diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h
> > --- a/src/http/ngx_http_request.h
> > +++ b/src/http/ngx_http_request.h
> > @@ -324,10 +324,6 @@ typedef struct {
> > #endif
> > #endif
> > 
> > -#if (NGX_HTTP_V3 || NGX_COMPAT)
> > -    ngx_http_v3_session_t            *v3_session;
> > -#endif
> > -
> >     ngx_chain_t                      *busy;
> >     ngx_int_t                         nbusy;
> > 
> > diff --git a/src/http/v3/ngx_http_v3.c b/src/http/v3/ngx_http_v3.c
> > --- a/src/http/v3/ngx_http_v3.c
> > +++ b/src/http/v3/ngx_http_v3.c
> > @@ -30,6 +30,8 @@ ngx_http_v3_init_session(ngx_connection_
> >         goto failed;
> >     }
> > 
> > +    h3c->http_connection = hc;
> > +
> >     ngx_queue_init(&h3c->blocked);
> > 
> >     h3c->keepalive.log = c->log;
> > @@ -48,7 +50,7 @@ ngx_http_v3_init_session(ngx_connection_
> >     cln->handler = ngx_http_v3_cleanup_session;
> >     cln->data = h3c;
> > 
> > -    hc->v3_session = h3c;
> > +    c->data = h3c;
> > 
> >     return NGX_OK;
> > 
> > diff --git a/src/http/v3/ngx_http_v3.h b/src/http/v3/ngx_http_v3.h
> > --- a/src/http/v3/ngx_http_v3.h
> > +++ b/src/http/v3/ngx_http_v3.h
> > @@ -78,11 +78,12 @@
> > #define NGX_HTTP_V3_ERR_DECODER_STREAM_ERROR       0x202
> > 
> > 
> > -#define ngx_http_quic_get_connection(c)                                       \
> > -    ((ngx_http_connection_t *) ((c)->quic ? (c)->quic->parent->data           \
> > +#define ngx_http_v3_get_session(c)                                            \
> > +    ((ngx_http_v3_session_t *) ((c)->quic ? (c)->quic->parent->data           \
> >                                           : (c)->data))
> > 
> > -#define ngx_http_v3_get_session(c)  ngx_http_quic_get_connection(c)->v3_session
> > +#define ngx_http_quic_get_connection(c)                                       \
> > +    (ngx_http_v3_get_session(c)->http_connection)
> > 
> > #define ngx_http_v3_get_module_loc_conf(c, module)                            \
> >     ngx_http_get_module_loc_conf(ngx_http_quic_get_connection(c)->conf_ctx,   \
> > @@ -120,6 +121,8 @@ struct ngx_http_v3_parse_s {
> > 
> > 
> > struct ngx_http_v3_session_s {
> > +    ngx_http_connection_t        *http_connection;
> > +
> >     ngx_http_v3_dynamic_table_t   table;
> > 
> >     ngx_event_t                   keepalive;
> > diff --git a/src/http/v3/ngx_http_v3_request.c b/src/http/v3/ngx_http_v3_request.c
> > --- a/src/http/v3/ngx_http_v3_request.c
> > +++ b/src/http/v3/ngx_http_v3_request.c
> > @@ -70,11 +70,6 @@ ngx_http_v3_init_stream(ngx_connection_t
> >     h3scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v3_module);
> > 
> >     if (c->quic == NULL) {
> > -        if (ngx_http_v3_init_session(c) != NGX_OK) {
> > -            ngx_http_close_connection(c);
> > -            return;
> > -        }
> > -
> >         h3scf->quic.idle_timeout = clcf->keepalive_timeout;
> >         ngx_quic_run(c, &h3scf->quic);
> >         return;
> > @@ -112,6 +107,10 @@ ngx_http_v3_init(ngx_connection_t *c)
> > 
> >     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http3 init");
> > 
> > +    if (ngx_http_v3_init_session(c) != NGX_OK) {
> > +        return NGX_ERROR;
> > +    }
> > +
> >     h3c = ngx_http_v3_get_session(c);
> >     clcf = ngx_http_v3_get_module_loc_conf(c, ngx_http_core_module);
> >     ngx_add_timer(&h3c->keepalive, clcf->keepalive_timeout);
> 
> -- 
> Sergey Kandaurov
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan