[PATCH 1 of 3] QUIC: reusable and idle modes for main connection

Sergey Kandaurov pluknet at nginx.com
Mon Jun 6 10:34:16 UTC 2022


> On 2 Jun 2022, at 17:52, Roman Arutyunyan <arut at nginx.com> wrote:
> 
> # HG changeset patch
> # User Roman Arutyunyan <arut at nginx.com>
> # Date 1652852691 -14400
> #      Wed May 18 09:44:51 2022 +0400
> # Branch quic
> # Node ID 617ec472690620cc75c473f97555138a4bc7d38b
> # Parent  a231f30606317a61efa3005fda9b43eb91d148d9
> QUIC: reusable and idle modes for main connection.
> 
> The modes are controlled by application layer.  For HTTP/3, they are enabled
> when there are no active request streams.
> 
> 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
> @@ -414,8 +414,8 @@ ngx_quic_input_handler(ngx_event_t *rev)
>     }
> 
>     if (c->close) {
> -        qc->error_reason = "graceful shutdown";
> -        ngx_quic_close_connection(c, NGX_OK);
> +        qc->error = NGX_QUIC_ERR_NO_ERROR;
> +        ngx_quic_close_connection(c, NGX_ERROR);

Now a valid connection is closed with INTERNAL_ERROR on graceful shutdown.
What's the real intention?

>         return;
>     }
> 
> 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
> @@ -161,6 +161,7 @@ ngx_quic_close_streams(ngx_connection_t 
>     ngx_pool_t         *pool;
>     ngx_queue_t        *q;
>     ngx_rbtree_t       *tree;
> +    ngx_connection_t   *sc;
>     ngx_rbtree_node_t  *node;
>     ngx_quic_stream_t  *qs;
> 
> @@ -185,24 +186,41 @@ ngx_quic_close_streams(ngx_connection_t 
>         return NGX_OK;
>     }
> 
> -#if (NGX_DEBUG)
> -    ns = 0;
> -#endif
> -
>     node = ngx_rbtree_min(tree->root, tree->sentinel);
> 
>     while (node) {
>         qs = (ngx_quic_stream_t *) node;
>         node = ngx_rbtree_next(tree, node);
> +        sc = qs->connection;
> +
> +        if (sc == NULL) {
> +            ngx_quic_close_stream(qs);
> +            continue;
> +        }
> +
> +        if (c->close || sc->idle || sc->reusable) {

I'm not sure how the sc->idle test is a worthwhile
as it seems never be set on HTTP/3 stream connections.

Originally, c->idle was used to close HTTP/1.x keep-alive
connections, and in HTTP/2 it is always set (see below).

Testing sc->reusable here is a moot point and looks like
an abuse of what when short on worker connections.
For tracking active streams, HTTP/2 uses h2c->processing.
If implemented same way in HTTP/3, sc->reusable becomes
unnecessary as well (not sure if that's possible).

> +            sc->close = 1;
> +            sc->read->handler(sc->read);
> +        }
> +    }
> +
> +    if (tree->root == tree->sentinel) {
> +        return NGX_OK;
> +    }
> +
> +#if (NGX_DEBUG)
> +    ns = 0;
> +#endif
> +
> +    for (node = ngx_rbtree_min(tree->root, tree->sentinel);
> +         node;
> +         node = ngx_rbtree_next(tree, node))
> +    {
> +        qs = (ngx_quic_stream_t *) node;
> 
>         qs->recv_state = NGX_QUIC_STREAM_RECV_RESET_RECVD;
>         qs->send_state = NGX_QUIC_STREAM_SEND_RESET_SENT;
> 
> -        if (qs->connection == NULL) {
> -            ngx_quic_close_stream(qs);
> -            continue;
> -        }
> -
>         ngx_quic_set_event(qs->connection->read);
>         ngx_quic_set_event(qs->connection->write);
> 
> @@ -587,6 +605,7 @@ ngx_quic_create_stream(ngx_connection_t 
> {
>     ngx_log_t              *log;
>     ngx_pool_t             *pool;
> +    ngx_uint_t              reusable;
>     ngx_queue_t            *q;
>     ngx_connection_t       *sc;
>     ngx_quic_stream_t      *qs;
> @@ -639,7 +658,13 @@ ngx_quic_create_stream(ngx_connection_t 
>     *log = *c->log;
>     pool->log = log;
> 
> +    reusable = c->reusable;
> +    ngx_reusable_connection(c, 0);
> +
>     sc = ngx_get_connection(c->fd, log);
> +
> +    ngx_reusable_connection(c, reusable);
> +
>     if (sc == NULL) {
>         ngx_destroy_pool(pool);
>         ngx_queue_insert_tail(&qc->streams.free, &qs->queue);
> 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
> @@ -210,6 +210,11 @@ ngx_http_v3_init_request_stream(ngx_conn
> 
>     h3c = ngx_http_v3_get_session(c);
> 
> +    if (!h3c->goaway && (ngx_terminate || ngx_exiting)) {
> +        h3c->goaway = 1;
> +        (void) ngx_http_v3_send_goaway(c, c->quic->id);
> +    }
> +
>     if (h3c->goaway) {
>         c->close = 1;
>         ngx_http_close_connection(c);
> @@ -258,7 +263,7 @@ ngx_http_v3_wait_request_handler(ngx_eve
>     size_t                     size;
>     ssize_t                    n;
>     ngx_buf_t                 *b;
> -    ngx_connection_t          *c;
> +    ngx_connection_t          *c, *pc;
>     ngx_pool_cleanup_t        *cln;
>     ngx_http_request_t        *r;
>     ngx_http_connection_t     *hc;
> @@ -385,6 +390,10 @@ ngx_http_v3_wait_request_handler(ngx_eve
>     h3c = ngx_http_v3_get_session(c);
>     h3c->nrequests++;
> 
> +    pc = c->quic->parent;
> +    pc->idle = 0;
> +    ngx_reusable_connection(pc, 0);
> +
>     if (h3c->keepalive.timer_set) {
>         ngx_del_timer(&h3c->keepalive);
>     }
> @@ -430,7 +439,7 @@ ngx_http_v3_cleanup_request(void *data)
> {
>     ngx_http_request_t  *r = data;
> 
> -    ngx_connection_t          *c;
> +    ngx_connection_t          *c, *pc;
>     ngx_http_v3_session_t     *h3c;
>     ngx_http_core_loc_conf_t  *clcf;
> 
> @@ -443,6 +452,16 @@ ngx_http_v3_cleanup_request(void *data)
>     h3c = ngx_http_v3_get_session(c);
> 
>     if (--h3c->nrequests == 0) {
> +        pc = c->quic->parent;
> +
> +        if (ngx_terminate || ngx_exiting) {
> +            ngx_quic_finalize_connection(pc, NGX_HTTP_V3_ERR_NO_ERROR, NULL);
> +            return;
> +        }
> +
> +        pc->idle = 1;
> +        ngx_reusable_connection(pc, 1);
> +

Such way it doesn't take any action when there are requests streams.
It could send GOAWAY proactively.  By the way, with the patch,
after graceful shutdown, it still allows processing of additional
new streams and rearms idle timeout, so it may never end.

HTTP/2 had set c->idle similarly before 5e95b9fb33b7.
Now it always sets c->idle and performs accordingly to the state.
Probably, HTTP/3 should be modified in somewhat the same direction.

>         clcf = ngx_http_v3_get_module_loc_conf(c, ngx_http_core_module);
>         ngx_add_timer(&h3c->keepalive, clcf->keepalive_timeout);
>     }
> diff --git a/src/http/v3/ngx_http_v3_uni.c b/src/http/v3/ngx_http_v3_uni.c
> --- a/src/http/v3/ngx_http_v3_uni.c
> +++ b/src/http/v3/ngx_http_v3_uni.c
> @@ -182,6 +182,11 @@ ngx_http_v3_uni_read_handler(ngx_event_t
> 
>     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http3 read handler");
> 
> +    if (c->close) {
> +        ngx_http_v3_close_uni_stream(c);
> +        return;
> +    }
> +
>     ngx_memzero(&b, sizeof(ngx_buf_t));
> 
>     while (rev->ready) {
> @@ -262,6 +267,11 @@ ngx_http_v3_uni_dummy_read_handler(ngx_e
> 
>     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http3 dummy read handler");
> 
> +    if (c->close) {
> +        ngx_http_v3_close_uni_stream(c);
> +        return;
> +    }
> +
>     if (rev->ready) {
>         if (c->recv(c, &ch, 1) != 0) {
>             ngx_http_v3_finalize_connection(c, NGX_HTTP_V3_ERR_NO_ERROR, NULL);
> 

-- 
Sergey Kandaurov



More information about the nginx-devel mailing list