Hi,

On Tue, Nov 29, 2022 at 06:00:40PM +0400, Roman Arutyunyan wrote:
> Hi,
>
> On Mon, Nov 28, 2022 at 09:40:05PM +0400, Sergey Kandaurov wrote:
> > On Thu, Nov 24, 2022 at 07:15:31PM +0400, Roman Arutyunyan wrote:
> > > Hi,
> > >
> > > On Thu, Oct 20, 2022 at 06:25:03PM +0400, Roman Arutyunyan wrote:
> > > > Hi,
> > > >
> > > > On Thu, Oct 20, 2022 at 03:50:15PM +0400, Sergey Kandaurov wrote:
> > > > > On Thu, Sep 08, 2022 at 01:06:35PM +0400, Roman Arutyunyan wrote:
> > > > > > # HG changeset patch
> > > > > > # User Roman Arutyunyan <arut@nginx.com>
> > > > > > # Date 1662627133 -14400
> > > > > > # Thu Sep 08 12:52:13 2022 +0400
> > > > > > # Branch quic
> > > > > > # Node ID e0634a484d9a2d82d43f565d64a0a22e989ac1cb
> > > > > > # Parent 1dd6fabfdcb5b52af495f9d8fc00f64ae36a537c
> > > > > > QUIC: idle mode for main connection.
> > > > > >
> > > > > > Now main QUIC connection for HTTP/3 always has c->idle flag set. This allows
> > > > > > the connection to receive worker shutdown notification. It is passed to
> > > > > > application level via a new conf->shutdown() callback.
> > > > > >
> > > > > > The HTTP/3 shutdown callback sends GOAWAY to client and gracefully shuts down
> > > > > > the QUIC connection.
> > > > > >
> > > > > > 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
> > > > > > @@ -341,6 +341,7 @@ ngx_quic_new_connection(ngx_connection_t
> > > > > > return NULL;
> > > > > > }
> > > > > >
> > > > > > + c->idle = 1;
> > > > > > ngx_reusable_connection(c, 1);
> > > > > >
> > > > > > ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > > > > > @@ -420,9 +421,9 @@ ngx_quic_input_handler(ngx_event_t *rev)
> > > > > > }
> > > > > >
> > > > > > if (c->close) {
> > > > > > - qc->error = NGX_QUIC_ERR_NO_ERROR;
> > > > > > - qc->error_reason = "graceful shutdown";
> > > > > > - ngx_quic_close_connection(c, NGX_ERROR);
> > > > > > + if (qc->conf->shutdown) {
> > > > >
> > > > > As previously discussed in private, this will need an additional check
> > > > > that we are not yet in qc->closing.
> > > > >
> > > > > > + qc->conf->shutdown(c);
> > > > > > + }
> > > > > > return;
> > > > > > }
> > > >
> > > >
> > > > Yes, added the check. Also, c->close is reset here similarly to HTTP/2
> > > > since we want to be able to handle future packets normally.
> > > >
> > > > Also, current code which closes the connection instantly should remain for
> > > > connection reuse. To tell reuse from shutdown we can check ngx_exiting.
> > > > Assuming reuse does not make sense in shutdown mode, this will work good.
> > > >
> > > > > > 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
> > > > > > @@ -28,6 +28,9 @@
> > > > > > #define NGX_QUIC_STREAM_UNIDIRECTIONAL 0x02
> > > > > >
> > > > > >
> > > > > > +typedef void (*ngx_quic_shutdown_pt)(ngx_connection_t *c);
> > > > > > +
> > > > > > +
> > > > > > typedef enum {
> > > > > > NGX_QUIC_STREAM_SEND_READY = 0,
> > > > > > NGX_QUIC_STREAM_SEND_SEND,
> > > > > > @@ -74,6 +77,8 @@ typedef struct {
> > > > > > ngx_int_t stream_reject_code_uni;
> > > > > > ngx_int_t stream_reject_code_bidi;
> > > > > >
> > > > > > + ngx_quic_shutdown_pt shutdown;
> > > > > > +
> > > > > > u_char av_token_key[NGX_QUIC_AV_KEY_LEN];
> > > > > > u_char sr_token_key[NGX_QUIC_SR_KEY_LEN];
> > > > > > } ngx_quic_conf_t;
> > > > > > diff --git a/src/http/v3/ngx_http_v3.h b/src/http/v3/ngx_http_v3.h
> > > > > > --- a/src/http/v3/ngx_http_v3.h
> > > > > > +++ b/src/http/v3/ngx_http_v3.h
> > > > > > @@ -141,6 +141,7 @@ struct ngx_http_v3_session_s {
> > > > > > uint64_t next_push_id;
> > > > > > uint64_t max_push_id;
> > > > > > uint64_t goaway_push_id;
> > > > > > + uint64_t next_request_id;
> > > > > >
> > > > > > off_t total_bytes;
> > > > > > off_t payload_bytes;
> > > > > > @@ -158,6 +159,7 @@ void ngx_http_v3_init(ngx_connection_t *
> > > > > > void ngx_http_v3_reset_connection(ngx_connection_t *c);
> > > > > > ngx_int_t ngx_http_v3_init_session(ngx_connection_t *c);
> > > > > > ngx_int_t ngx_http_v3_check_flood(ngx_connection_t *c);
> > > > > > +void ngx_http_v3_shutdown(ngx_connection_t *c);
> > > > > >
> > > > > > ngx_int_t ngx_http_v3_read_request_body(ngx_http_request_t *r);
> > > > > > ngx_int_t ngx_http_v3_read_unbuffered_request_body(ngx_http_request_t *r);
> > > > > > 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
> > > > > > @@ -249,6 +249,8 @@ ngx_http_v3_create_srv_conf(ngx_conf_t *
> > > > > > h3scf->quic.stream_reject_code_bidi = NGX_HTTP_V3_ERR_REQUEST_REJECTED;
> > > > > > h3scf->quic.active_connection_id_limit = NGX_CONF_UNSET_UINT;
> > > > > >
> > > > > > + h3scf->quic.shutdown = ngx_http_v3_shutdown;
> > > > > > +
> > > > > > return h3scf;
> > > > > > }
> > > > > >
> > > > > > 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
> > > > > > @@ -97,6 +97,37 @@ ngx_http_v3_init(ngx_connection_t *c)
> > > > > > }
> > > > > >
> > > > > >
> > > > > > +void
> > > > > > +ngx_http_v3_shutdown(ngx_connection_t *c)
> > > > > > +{
> > > > > > + ngx_http_v3_session_t *h3c;
> > > > >
> > > > > extra indent
> > > > >
> > > > > > +
> > > > > > + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http3 shutdown");
> > > > > > +
> > > > > > + h3c = ngx_http_v3_get_session(c);
> > > > > > +
> > > > > > + if (h3c == NULL) {
> > > > > > + ngx_quic_finalize_connection(c, NGX_HTTP_V3_ERR_NO_ERROR,
> > > > > > + "connection shutdown");
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + if (!h3c->goaway) {
> > > > > > + h3c->goaway = 1;
> > > > > > +
> > > > > > +#if (NGX_HTTP_V3_HQ)
> > > > > > + if (!h3c->hq)
> > > > > > +#endif
> > > > > > + {
> > > > > > + (void) ngx_http_v3_send_goaway(c, h3c->next_request_id);
> > > > > > + }
> > > > > > +
> > > > > > + ngx_http_v3_shutdown_connection(c, NGX_HTTP_V3_ERR_NO_ERROR,
> > > > > > + "connection shutdown");
> > > > > > + }
> > > > >
> > > > > Note that this callback is used to be called from a read event as part of
> > > > > graceful shutdown.
> > > > > With ngx_quic_finalize_connection() remade in patch #4 (reusable mode)
> > > > > to defer closing QUIC connection to a posted event, this call now results
> > > > > in a posted event, which no one can fulfill, hence no further action until
> > > > > quic idle timeout fires.
> > > > > It could be fixed by executing known posted events after shutdown callback
> > > > > or more globally - as part of graceful shutdown itself.
> > > >
> > > > Yes, events posted from ngx_close_idle_connections() are not handled right away.
> > > > Instead, they are handled at the end of the next cycle, which normally
> > > > happens after a timeout. There seems to be no pretty way to fix this, unless
> > > > we handle posted events in ngx_worker_process_cycle() right after
> > > > ngx_close_idle_connections(). We are trying to avoid global changes like this.
> > > >
> > > > I suggest posting current connection read event as a next posted event. This
> > > > will effectively set next cycle timeout to be zero and eliminate the problem.
> > > >
> > > > > > +}
> > > > > > +
> > > > > > +
> > > > > > static void
> > > > > > ngx_http_v3_init_request_stream(ngx_connection_t *c)
> > > > > > {
> > > > > > @@ -137,6 +168,8 @@ ngx_http_v3_init_request_stream(ngx_conn
> > > > > >
> > > > > > pc = c->quic->parent;
> > > > > >
> > > > > > + h3c->next_request_id = c->quic->id + 0x04;
> > > > > > +
> > > > > > if (n + 1 == clcf->keepalive_requests
> > > > > > || ngx_current_msec - pc->start_time > clcf->keepalive_time)
> > > > > > {
> > > > > > @@ -146,7 +179,7 @@ ngx_http_v3_init_request_stream(ngx_conn
> > > > > > if (!h3c->hq)
> > > > > > #endif
> > > > > > {
> > > > > > - if (ngx_http_v3_send_goaway(c, (n + 1) << 2) != NGX_OK) {
> > > > > > + if (ngx_http_v3_send_goaway(c, h3c->next_request_id) != NGX_OK) {
> > > > > > ngx_http_close_connection(c);
> > > > > > return;
> > > > > > }
> > > > > >
> > > > > > _______________________________________________
> > > > > > 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
> > > >
> > > > Attached is a diff to the current patch.
> > > >
> > > > --
> > > > Roman
> > >
> > > > # HG changeset patch
> > > > # User Roman Arutyunyan <arut@nginx.com>
> > > > # Date 1666273166 -14400
> > > > # Thu Oct 20 17:39:26 2022 +0400
> > > > # Branch quic
> > > > # Node ID d6c725081a0b024886822e1cc722fdace9c32621
> > > > # Parent a4ba2ac5fa55ef94bb75a66e66e0b19d792fed10
> > > > [mq]: quic-idle-fix1
> > > >
> > > > 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
> > > > @@ -421,9 +421,22 @@ ngx_quic_input_handler(ngx_event_t *rev)
> > > > }
> > > >
> > > > if (c->close) {
> > > > - if (qc->conf->shutdown) {
> > > > + c->close = 0;
> > > > +
> > > > + if (!ngx_exiting) {
> > > > + qc->error = NGX_QUIC_ERR_NO_ERROR;
> > > > + qc->error_reason = "graceful shutdown";
> > > > + ngx_quic_close_connection(c, NGX_ERROR);
> > > > + return;
> > > > + }
> > > > +
> > > > + if (!qc->closing && qc->conf->shutdown) {
> > > > + /* do not delay events posted by shutdown() */
> > > > + ngx_post_event(rev, &ngx_posted_next_events);
> > >
> > > Following the change http://hg.nginx.org/nginx/rev/b809f53d3f5b, this part
> > > is no longer needed. The new diff attached.
> >
> > Note that adding support for "idle mode for main connection" combined
> > with b809f53d3f5b uncovers a socket leak due to qc->close.cancelable set.
> >
> > In more details:
> > - there can be connections with stretched pto e.g. due to missing client ack,
> > large enough such that a close timer (3*pto) fires after the idle timer
> > - now that QUIC connection is handled on graceful shutdown as idle,
> > entering graceful shutdown results in setting a close timer, and closing
> > can take additional cycle following the "qc->close.timer_set" condition
> > - pto and idle timers fired (and removed)
> > - only close and, possibly, keepalive timers left; both cancelable
> >
> > I suggest removing the cancelable property of a close timer
> > given that we want to wait for 3*PTO while in a closing state
> > and send back sensible CONNECTION_CLOSE to any further input
> > (otherwise it would emit less sensible stateless reset).
> > This will fix leaving event cycle without non-cancelable timers set.
>
> Setting "cancelable" for a client-related entity looks wrong both in QUIC and
> HTTP/3. IMHO it should be removed, see attach.
>
> > > > +
> > > > qc->conf->shutdown(c);
> > > > }
> > > > +
> > > > return;
> > > > }
> > >
> > >
> > > --
> > > Roman Arutyunyan
> >
> > > # HG changeset patch
> > > # User Roman Arutyunyan <arut@nginx.com>
> > > # Date 1669302768 -14400
> > > # Thu Nov 24 19:12:48 2022 +0400
> > > # Branch quic
> > > # Node ID ec21165abb4333f77fb85e956cccf300c8ae1acf
> > > # Parent 6fba6061b65f0ee41c231bde6b738cfae00bb179
> > > [mq]: quic-idle-fix1
> > >
> > > 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
> > > @@ -421,9 +421,19 @@ ngx_quic_input_handler(ngx_event_t *rev)
> > > }
> > >
> > > if (c->close) {
> > > - if (qc->conf->shutdown) {
> > > + c->close = 0;
> > > +
> > > + if (!ngx_exiting) {
> > > + qc->error = NGX_QUIC_ERR_NO_ERROR;
> > > + qc->error_reason = "graceful shutdown";
> > > + ngx_quic_close_connection(c, NGX_ERROR);
> > > + return;
> > > + }
> > > +
> > > + if (!qc->closing && qc->conf->shutdown) {
> > > qc->conf->shutdown(c);
> > > }
> > > +
> > > return;
> > > }
> > >
> > > 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
> > > @@ -101,7 +101,7 @@ ngx_http_v3_init(ngx_connection_t *c)
> > > void
> > > ngx_http_v3_shutdown(ngx_connection_t *c)
> > > {
> > > - ngx_http_v3_session_t *h3c;
> > > + ngx_http_v3_session_t *h3c;
> > >
> > > ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http3 shutdown");
> > >
> >
> > Looks good.
> > _______________________________________________
> > nginx-devel mailing list -- nginx-devel@nginx.org
> > To unsubscribe send an email to nginx-devel-leave@nginx.org

> # HG changeset patch
> # User Roman Arutyunyan <arut@nginx.com>
> # Date 1669730306 -14400
> # Tue Nov 29 17:58:26 2022 +0400
> # Branch quic
> # Node ID 174c09dedc046aac2ca5308ab8957fc61b27c778
> # Parent 576d9decab8bdb34a85b0cf52acf3afb3ae97b8d
> imported patch quic-idle-fix2
>
> 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
> @@ -275,22 +275,18 @@ ngx_quic_new_connection(ngx_connection_t
> qc->pto.log = c->log;
> qc->pto.data = c;
> qc->pto.handler = ngx_quic_pto_handler;
> - qc->pto.cancelable = 1;
>
> qc->push.log = c->log;
> qc->push.data = c;
> qc->push.handler = ngx_quic_push_handler;
> - qc->push.cancelable = 1;
>
> qc->close.log = c->log;
> qc->close.data = c;
> qc->close.handler = ngx_quic_close_handler;
> - qc->close.cancelable = 1;
>
> qc->path_validation.log = c->log;
> qc->path_validation.data = c;
> qc->path_validation.handler = ngx_quic_path_validation_handler;
> - qc->path_validation.cancelable = 1;
>
> qc->conf = conf;
>
> diff --git a/src/http/v3/ngx_http_v3.c b/src/http/v3/ngx_http_v3.c
> --- a/src/http/v3/ngx_http_v3.c
> +++ b/src/http/v3/ngx_http_v3.c
> @@ -55,7 +55,6 @@ ngx_http_v3_init_session(ngx_connection_
> h3c->keepalive.log = pc->log;
> h3c->keepalive.data = pc;
> h3c->keepalive.handler = ngx_http_v3_keepalive_handler;
> - h3c->keepalive.cancelable = 1;
>
> h3c->table.send_insert_count.log = pc->log;
> h3c->table.send_insert_count.data = pc;

Full patch.

--
Roman

# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1669802948 -14400
# Wed Nov 30 14:09:08 2022 +0400
# Branch quic
# Node ID 97844d0c98127588932f04948b4ef0a144fd5826
# Parent f9f8bdc571ea0a0b5cc1dc97e3c168db37d32c67
QUIC: removed cancelable flag from QUIC and HTTP/3 events.

All these events are created in context of a client connection and are deleted
when the connection is closed. Setting ev->cancelable could trigger premature
connection closure and a socket leak alert.

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
@@ -275,22 +275,18 @@ ngx_quic_new_connection(ngx_connection_t
qc->pto.log = c->log;
qc->pto.data = c;
qc->pto.handler = ngx_quic_pto_handler;
- qc->pto.cancelable = 1;

qc->push.log = c->log;
qc->push.data = c;
qc->push.handler = ngx_quic_push_handler;
- qc->push.cancelable = 1;

qc->close.log = c->log;
qc->close.data = c;
qc->close.handler = ngx_quic_close_handler;
- qc->close.cancelable = 1;

qc->path_validation.log = c->log;
qc->path_validation.data = c;
qc->path_validation.handler = ngx_quic_path_validation_handler;
- qc->path_validation.cancelable = 1;

qc->conf = conf;

diff --git a/src/http/v3/ngx_http_v3.c b/src/http/v3/ngx_http_v3.c
--- a/src/http/v3/ngx_http_v3.c
+++ b/src/http/v3/ngx_http_v3.c
@@ -55,7 +55,6 @@ ngx_http_v3_init_session(ngx_connection_
h3c->keepalive.log = pc->log;
h3c->keepalive.data = pc;
h3c->keepalive.handler = ngx_http_v3_keepalive_handler;
- h3c->keepalive.cancelable = 1;

h3c->table.send_insert_count.log = pc->log;
h3c->table.send_insert_count.data = pc;
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org