On Mon, May 30, 2022 at 05:54:34PM +0400, Sergey Kandaurov wrote:
>
> > On 18 May 2022, at 10:57, Roman Arutyunyan <arut@nginx.com> wrote:
> >
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1652852691 -14400
> > # Wed May 18 09:44:51 2022 +0400
> > # Branch quic
> > # Node ID 67ae4b649f2e38a44b245b7a842cf396c8250f02
> > # Parent c2f5d79cde64457f1fa7344c56a5248a677a7e46
> > 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);
> > 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) {
> > + 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);
> > +
>
> jftr, this pollutes debug log a bit, but generally looks ok.
>
> > 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 (ngx_terminate || ngx_exiting) {
> > + h3c->goaway = 1;
> > + (void) ngx_http_v3_send_goaway(c, c->quic->id);
> > + }
> > +
>
> This results in ever increasing GOAWAY identifiers if multiple
> new client streams received after graceful shutdown initiated.
> This is forbidden per rfc9114, 5.2. Connection Shutdown.
>
> I wonder if it should move after the below condition.

We cannot move it below since we need the below condition to work in case
of (ngx_terminating || ngx_exiting) too.

I suggest that we add !h3c->goaway to the new condition.

> > 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);
> > +
> > 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
>
> _______________________________________________
> nginx-devel mailing list -- nginx-devel@nginx.org
> To unsubscribe send an email to nginx-devel-leave@nginx.org
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org