On Mon, Jun 06, 2022 at 02:34:16PM +0400, Sergey Kandaurov wrote:
>
> > On 2 Jun 2022, at 17:52, 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 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?

The intention is to close the QUIC connection while in this call. Closing
it with NGX_OK will delay the actual closure. Closing with NGX_DONE will
do a silent closure (no CC sent). The only option left is NGX_ERROR.

Sadly, ngx_quic_close_connection() treats NGX_QUIC_ERR_NO_ERROR as a missing
error code and indeed changes it to INTERNAL_ERROR. I think we need another
code for a missing error.

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
@@ -317,6 +317,8 @@ ngx_quic_new_connection(ngx_connection_t
qc->congestion.ssthresh = (size_t) -1;
qc->congestion.recovery_start = ngx_current_msec;

+ qc->error = (ngx_uint_t) -1;
+
if (pkt->validated && pkt->retried) {
qc->tp.retry_scid.len = pkt->dcid.len;
qc->tp.retry_scid.data = ngx_pstrdup(c->pool, &pkt->dcid);
@@ -523,7 +525,7 @@ ngx_quic_close_connection(ngx_connection
qc->error = NGX_QUIC_ERR_NO_ERROR;

} else {
- if (qc->error == 0 && !qc->error_app) {
+ if (qc->error == (ngx_uint_t) -1 && !qc->error_app) {
qc->error = NGX_QUIC_ERR_INTERNAL_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) {
>
> 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).

QUIC layer is not (and should never be) aware of the application protocol.
Here we can only rely on general conventions for connection flags. c->idle and
c->reusable differ in degrees of gracefullness of shutting down the connection.
c->reusable is a less graceful version of c->idle. But in both cases the
connection can be quickly terminated by setting c->close and calling read
handler, which is exactly what we want in this funtcion.

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

There's an additional check for that when a new stream is created. HTTP/3
keepalive is not rearmed for such streams and GOAWAY is sent, although it's
not proactive.

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

The problem with this approach is QUIC/HTTP layers separation. If we set
c->idle, a graceful shutdown notification will arrive to QUIC.
However, GOAWAY cannot be sent from QUIC, because it's an HTTP/3-level command.
We can introduce a callback for this. As similar callback is added by the
next pacth anyway.

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