On Tue, Sep 12, 2023 at 05:04:59PM +0400, Sergey Kandaurov wrote:
>
> > On 11 Sep 2023, at 15:30, Roman Arutyunyan <arut@nginx.com> wrote:
> >
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1694415935 -14400
> > # Mon Sep 11 11:05:35 2023 +0400
> > # Node ID 65574b876c4047495ac8622c6161fc87c1b75913
> > # Parent daf8f5ba23d8e9955b22782d945f9c065f4b6baa
> > QUIC: "handshake_timeout" configuration parameter.
> >
> > Previously QUIC did not have such parameter and handshake duration was
> > controlled by HTTP/3. However that required creating and storing HTTP/3
> > session while processing the first client datagram. Apparently there's
>
> s/while/before ?

I'd still keep the "while". However, since apparently it sounds confusing,
I have rephrased it a little.

> > no convenient way to store the session object before QUIC handshake. In
> > the following patch session creation will be postponed until first stream
> > creation.
> >
> > diff --git a/src/event/quic/ngx_event_quic.c b/src/event/quic/ngx_event_quic.c
> > --- a/src/event/quic/ngx_event_quic.c
> > +++ b/src/event/quic/ngx_event_quic.c
> > @@ -213,6 +213,8 @@ ngx_quic_run(ngx_connection_t *c, ngx_qu
> > ngx_add_timer(c->read, qc->tp.max_idle_timeout);
> > ngx_quic_connstate_dbg(c);
> >
> > + ngx_add_timer(&qc->close, qc->conf->handshake_timeout);
> > +
>
> It makes sense to cap handshake_timeout to idle timeout.
> I.e. ngx_min(qc->conf->handshake_timeout, qc->tp.max_idle_timeout);
>
> Or additionally handle this in ngx_quic_close_connection(),
> by removing the timer if set for the NGX_DONE case.

I like this option better. In fact, we can remove it once for both cases.

> Otherwise the connection would prolong unnecessarily on idle timeout
> for no purpose, because close timer set prevents closing a connection.
> As a (somewhat degenerate) ready example, see h3_keepalive.t TODO
> with too long connection close after the patch applied.
> The test uses "keepalive_timeout 0;" in the configuration.
>
> Also I would move setting close timer before calling
> ngx_quic_connstate_dbg() to reflect it's set in the debug log.

OK

> > c->read->handler = ngx_quic_input_handler;
> >
> > return;
> > @@ -521,6 +523,10 @@ ngx_quic_close_connection(ngx_connection
> > qc->error_app ? "app " : "", qc->error,
> > qc->error_reason ? qc->error_reason : "");
> >
> > + if (qc->close.timer_set) {
> > + ngx_del_timer(&qc->close);
> > + }
> > +
>
> Removing timer there makes the check below for the timer set unnecessary:
>
> : if (rc == NGX_OK && !qc->close.timer_set) {

Now it seems to be we don't need this condition at all and it has nothing to
do with the patch. Normally, we'd like to use a timeout for the lower level.
However, the PTO() value is only different for the application level due to
max_ack_delay and only after handshake completion. However, as discussed
before, after handshake completion other levels are cleared. Also, resetting
the timer for the same value has no performance implications.

So we can remove the condition in a separate patch.

> > for (i = 0; i < NGX_QUIC_SEND_CTX_LAST; i++) {
> > ctx = &qc->send_ctx[i];
> >
> > diff --git a/src/event/quic/ngx_event_quic.h b/src/event/quic/ngx_event_quic.h
> > --- a/src/event/quic/ngx_event_quic.h
> > +++ b/src/event/quic/ngx_event_quic.h
> > @@ -67,7 +67,8 @@ typedef struct {
> > ngx_flag_t retry;
> > ngx_flag_t gso_enabled;
> > ngx_flag_t disable_active_migration;
> > - ngx_msec_t timeout;
> > + ngx_msec_t handshake_timeout;
> > + ngx_msec_t idle_timeout;
> > ngx_str_t host_key;
> > size_t stream_buffer_size;
> > ngx_uint_t max_concurrent_streams_bidi;
> > diff --git a/src/event/quic/ngx_event_quic_streams.c b/src/event/quic/ngx_event_quic_streams.c
> > --- a/src/event/quic/ngx_event_quic_streams.c
> > +++ b/src/event/quic/ngx_event_quic_streams.c
> > @@ -630,6 +630,10 @@ ngx_quic_do_init_streams(ngx_connection_
> >
> > qc->streams.initialized = 1;
> >
> > + if (!qc->closing && qc->close.timer_set) {
> > + ngx_del_timer(&qc->close);
> > + }
> > +
> > return NGX_OK;
> > }
> >
> > diff --git a/src/event/quic/ngx_event_quic_transport.c b/src/event/quic/ngx_event_quic_transport.c
> > --- a/src/event/quic/ngx_event_quic_transport.c
> > +++ b/src/event/quic/ngx_event_quic_transport.c
> > @@ -1985,7 +1985,7 @@ ngx_quic_init_transport_params(ngx_quic_
> > * tp->preferred_address = NULL
> > */
> >
> > - tp->max_idle_timeout = qcf->timeout;
> > + tp->max_idle_timeout = qcf->idle_timeout;
> >
> > tp->max_udp_payload_size = NGX_QUIC_MAX_UDP_PAYLOAD_SIZE;
> >
> > diff --git a/src/http/v3/ngx_http_v3_module.c b/src/http/v3/ngx_http_v3_module.c
> > --- a/src/http/v3/ngx_http_v3_module.c
> > +++ b/src/http/v3/ngx_http_v3_module.c
> > @@ -192,7 +192,7 @@ ngx_http_v3_create_srv_conf(ngx_conf_t *
> > * h3scf->quic.host_key = { 0, NULL }
> > * h3scf->quic.stream_reject_code_uni = 0;
> > * h3scf->quic.disable_active_migration = 0;
> > - * h3scf->quic.timeout = 0;
> > + * h3scf->quic.idle_timeout = 0;
> > * h3scf->max_blocked_streams = 0;
> > */
> >
> > @@ -223,7 +223,8 @@ ngx_http_v3_merge_srv_conf(ngx_conf_t *c
> > ngx_http_v3_srv_conf_t *prev = parent;
> > ngx_http_v3_srv_conf_t *conf = child;
> >
> > - ngx_http_ssl_srv_conf_t *sscf;
> > + ngx_http_ssl_srv_conf_t *sscf;
> > + ngx_http_core_srv_conf_t *cscf;
> >
> > ngx_conf_merge_value(conf->enable, prev->enable, 1);
> >
> > @@ -281,6 +282,9 @@ ngx_http_v3_merge_srv_conf(ngx_conf_t *c
> > return NGX_CONF_ERROR;
> > }
> >
> > + cscf = ngx_http_conf_get_module_srv_conf(cf, ngx_http_core_module);
> > + conf->quic.handshake_timeout = cscf->client_header_timeout;
> > +
> > sscf = ngx_http_conf_get_module_srv_conf(cf, ngx_http_ssl_module);
> > conf->quic.ssl = &sscf->ssl;
> >
> > 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
> > @@ -58,18 +58,15 @@ static const struct {
> > void
> > ngx_http_v3_init_stream(ngx_connection_t *c)
> > {
> > - ngx_http_v3_session_t *h3c;
> > ngx_http_connection_t *hc, *phc;
> > ngx_http_v3_srv_conf_t *h3scf;
> > ngx_http_core_loc_conf_t *clcf;
> > - ngx_http_core_srv_conf_t *cscf;
> >
> > hc = c->data;
> >
> > hc->ssl = 1;
> >
> > clcf = ngx_http_get_module_loc_conf(hc->conf_ctx, ngx_http_core_module);
> > - cscf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_core_module);
> > h3scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v3_module);
>
> Not directly related, h3scf initialization can be moved
> under the below condition where it is only used.
>
> >
> > if (c->quic == NULL) {
> > @@ -78,10 +75,7 @@ ngx_http_v3_init_stream(ngx_connection_t
> > return;
> > }
> >
> > - h3c = hc->v3_session;
> > - ngx_add_timer(&h3c->keepalive, cscf->client_header_timeout);
> > -
> > - h3scf->quic.timeout = clcf->keepalive_timeout;
> > + h3scf->quic.idle_timeout = clcf->keepalive_timeout;
> > ngx_quic_run(c, &h3scf->quic);
> > return;
> > }
>
> --
> Sergey Kandaurov
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel