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

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

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.

> 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) {

> 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