> On 11 Nov 2021, at 00:42, Roman Arutyunyan <[email protected]> wrote: > > On Wed, Nov 10, 2021 at 03:59:39PM +0300, Sergey Kandaurov wrote: >> >>> On 18 Oct 2021, at 15:48, Roman Arutyunyan <[email protected]> wrote: >>> >>> # HG changeset patch >>> # User Roman Arutyunyan <[email protected]> >>> # Date 1634561226 -10800 >>> # Mon Oct 18 15:47:06 2021 +0300 >>> # Branch quic >>> # Node ID 8ae53c592c719af4f3ba47dbd85f78be27aaf7db >>> # Parent 8739f475583031399879ef0af2eb5af76008449e >>> HTTP/3: allowed QUIC stream connection reuse. >>> >>> A QUIC stream connection is treated as reusable until first bytes of request >>> arrive, which is also when the request object is now allocated. A >>> connection >>> closed as a result of draining, is reset with the error code >>> H3_REQUEST_REJECTED. Such behavior is allowed by quic-http-34: >>> >>> Once a request stream has been opened, the request MAY be cancelled >>> by either endpoint. Clients cancel requests if the response is no >>> longer of interest; servers cancel requests if they are unable to or >>> choose not to respond. >>> >>> When the server cancels a request without performing any application >>> processing, the request is considered "rejected." The server SHOULD >>> abort its response stream with the error code H3_REQUEST_REJECTED. >>> >>> The client can treat requests rejected by the server as though they had >>> never been sent at all, thereby allowing them to be retried later. >>> >> >> Looks good. See below for minor comments. >> BTW, if we still hit the worker_connections limit, this leads to >> an entire QUIC connection close, but I doubt we can easily improve this. > > When there's not enough worker_connections for a new QUIC stream, we can > send H3_REQUEST_REJECTED to client without creating a stream. We can discuss > this later. > >>> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c >>> --- a/src/http/ngx_http_request.c >>> +++ b/src/http/ngx_http_request.c >>> @@ -3743,15 +3743,14 @@ ngx_http_free_request(ngx_http_request_t >>> >>> log->action = "closing request"; >>> >>> - if (r->connection->timedout) { >>> + if (r->connection->timedout >>> +#if (NGX_HTTP_QUIC) >>> + && r->connection->quic == NULL >>> +#endif >>> + ) >>> + { >>> clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); >>> >>> -#if (NGX_HTTP_V3) >>> - if (r->connection->quic) { >>> - (void) ngx_quic_reset_stream(r->connection, >>> - >>> NGX_HTTP_V3_ERR_GENERAL_PROTOCOL_ERROR); >>> - } else >>> -#endif >>> if (clcf->reset_timedout_connection) { >>> linger.l_onoff = 1; >>> linger.l_linger = 0; >>> @@ -3763,14 +3762,6 @@ ngx_http_free_request(ngx_http_request_t >>> "setsockopt(SO_LINGER) failed"); >>> } >>> } >>> - >>> - } else if (!r->response_sent) { >>> -#if (NGX_HTTP_V3) >>> - if (r->connection->quic) { >>> - (void) ngx_quic_reset_stream(r->connection, >>> - NGX_HTTP_V3_ERR_INTERNAL_ERROR); >>> - } >>> -#endif >>> } >>> >>> /* the various request strings were allocated from r->pool */ >>> @@ -3830,6 +3821,12 @@ ngx_http_close_connection(ngx_connection >>> >>> #endif >>> >>> +#if (NGX_HTTP_V3) >>> + if (ngx_http_v3_connection(c)) { >>> + ngx_http_v3_reset_connection(c); >>> + } >>> +#endif >>> + >>> #if (NGX_STAT_STUB) >>> (void) ngx_atomic_fetch_add(ngx_stat_active, -1); >>> #endif >>> 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 >>> @@ -90,6 +90,9 @@ >>> #define ngx_http_v3_shutdown_connection(c, code, reason) >>> \ >>> ngx_quic_shutdown_connection(c->quic->parent, code, reason) >>> >>> +#define ngx_http_v3_connection(c) >>> \ >>> + ((c)->quic ? ngx_http_quic_get_connection(c)->addr_conf->http3 : 0) >>> + >>> >>> typedef struct { >>> size_t max_table_capacity; >>> @@ -138,6 +141,7 @@ struct ngx_http_v3_session_s { >>> >>> >>> void ngx_http_v3_init(ngx_connection_t *c); >>> +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); >>> >>> 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 >>> @@ -10,6 +10,7 @@ >>> #include <ngx_http.h> >>> >>> >>> +static void ngx_http_v3_wait_request_handler(ngx_event_t *rev); >>> static void ngx_http_v3_cleanup_request(void *data); >>> static void ngx_http_v3_process_request(ngx_event_t *rev); >>> static ngx_int_t ngx_http_v3_process_header(ngx_http_request_t *r, >>> @@ -53,12 +54,8 @@ static const struct { >>> void >>> ngx_http_v3_init(ngx_connection_t *c) >>> { >>> - size_t size; >>> uint64_t n; >>> - ngx_buf_t *b; >>> ngx_event_t *rev; >>> - ngx_pool_cleanup_t *cln; >>> - ngx_http_request_t *r; >>> ngx_http_connection_t *hc; >>> ngx_http_v3_session_t *h3c; >>> ngx_http_core_loc_conf_t *clcf; >>> @@ -96,7 +93,7 @@ ngx_http_v3_init(ngx_connection_t *c) >>> h3c = ngx_http_v3_get_session(c); >>> >>> if (h3c->goaway) { >>> - ngx_quic_reset_stream(c, NGX_HTTP_V3_ERR_REQUEST_REJECTED); >>> + c->close = 1; >>> ngx_http_close_connection(c); >>> return; >>> } >>> @@ -116,21 +113,57 @@ ngx_http_v3_init(ngx_connection_t *c) >>> "reached maximum number of >>> requests"); >>> } >>> >>> - cln = ngx_pool_cleanup_add(c->pool, 0); >>> - if (cln == NULL) { >>> + rev = c->read; >>> + rev->handler = ngx_http_v3_wait_request_handler; >>> + c->write->handler = ngx_http_empty_handler; >>> + >>> + if (rev->ready) { >>> + rev->handler(rev); >>> + return; >>> + } >>> + >>> + cscf = ngx_http_get_module_srv_conf(hc->conf_ctx, >>> ngx_http_core_module); >>> + >>> + ngx_add_timer(rev, cscf->client_header_timeout); >>> + ngx_reusable_connection(c, 1); >>> + >>> + if (ngx_handle_read_event(rev, 0) != NGX_OK) { >>> + ngx_http_close_connection(c); >>> + return; >>> + } >>> +} >>> + >>> + >>> +static void >>> +ngx_http_v3_wait_request_handler(ngx_event_t *rev) >>> +{ >>> + size_t size; >>> + ssize_t n; >>> + ngx_buf_t *b; >>> + ngx_connection_t *c; >>> + ngx_pool_cleanup_t *cln; >>> + ngx_http_request_t *r; >>> + ngx_http_connection_t *hc; >>> + ngx_http_v3_session_t *h3c; >>> + ngx_http_core_srv_conf_t *cscf; >>> + >>> + c = rev->data; >>> + >>> + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http3 wait request >>> handler"); >>> + >>> + if (rev->timedout) { >>> + ngx_log_error(NGX_LOG_INFO, c->log, NGX_ETIMEDOUT, "client timed >>> out"); >>> + c->timedout = 1; >>> ngx_http_close_connection(c); >>> return; >>> } >>> >>> - cln->handler = ngx_http_v3_cleanup_request; >>> - cln->data = c; >>> - >>> - h3c->nrequests++; >>> - >>> - if (h3c->keepalive.timer_set) { >>> - ngx_del_timer(&h3c->keepalive); >>> + if (c->close) { >>> + ngx_http_close_connection(c); >>> + return; >>> } >>> >>> + hc = c->data; >>> cscf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_core_module); >>> >>> size = cscf->client_header_buffer_size; >>> @@ -159,8 +192,49 @@ ngx_http_v3_init(ngx_connection_t *c) >>> b->end = b->last + size; >>> } >>> >>> + n = c->recv(c, b->last, size); >>> + >>> + if (n == NGX_AGAIN) { >>> + >>> + if (!rev->timer_set) { >>> + ngx_add_timer(rev, cscf->client_header_timeout); >>> + ngx_reusable_connection(c, 1); >>> + } >>> + >>> + if (ngx_handle_read_event(rev, 0) != NGX_OK) { >>> + ngx_http_close_connection(c); >>> + return; >>> + } >>> + >>> + /* >>> + * We are trying to not hold c->buffer's memory for an idle >>> connection. >>> + */ >>> + >>> + if (ngx_pfree(c->pool, b->start) == NGX_OK) { >>> + b->start = NULL; >>> + } >>> + >>> + return; >>> + } >>> + >>> + if (n == NGX_ERROR) { >>> + ngx_http_close_connection(c); >>> + return; >>> + } >>> + >>> + if (n == 0) { >>> + ngx_log_error(NGX_LOG_INFO, c->log, 0, >>> + "client closed connection"); >>> + ngx_http_close_connection(c); >>> + return; >>> + } >>> + >>> + b->last += n; >>> + >>> c->log->action = "reading client request"; >>> >>> + ngx_reusable_connection(c, 0); >>> + >>> r = ngx_http_create_request(c); >>> if (r == NULL) { >>> ngx_http_close_connection(c); >>> @@ -171,7 +245,7 @@ ngx_http_v3_init(ngx_connection_t *c) >>> >>> r->v3_parse = ngx_pcalloc(r->pool, sizeof(ngx_http_v3_parse_t)); >>> if (r->v3_parse == NULL) { >>> - ngx_http_close_connection(c); >>> + ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); >>> return; >>> } >>> >> >> Since we now defer request initialization until first bytes, that's fine. >> >>> @@ -179,23 +253,59 @@ ngx_http_v3_init(ngx_connection_t *c) >>> * cscf->large_client_header_buffers.num; >>> >>> c->data = r; >>> - c->requests = n + 1; >>> + c->requests = (c->quic->id >> 2) + 1; >>> + >>> + cln = ngx_pool_cleanup_add(r->pool, 0); >>> + if (cln == NULL) { >>> + ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR); >>> + return; >>> + } >>> + >>> + cln->handler = ngx_http_v3_cleanup_request; >>> + cln->data = r; >>> + >>> + h3c = ngx_http_v3_get_session(c); >>> + h3c->nrequests++; >>> + >>> + if (h3c->keepalive.timer_set) { >>> + ngx_del_timer(&h3c->keepalive); >>> + } >>> >>> - rev = c->read; >>> rev->handler = ngx_http_v3_process_request; >>> + ngx_http_v3_process_request(rev); >>> +} >>> >>> - ngx_http_v3_process_request(rev); >>> + >>> +void >>> +ngx_http_v3_reset_connection(ngx_connection_t *c) >>> +{ >>> + if (c->timedout) { >>> + ngx_quic_reset_stream(c, NGX_HTTP_V3_ERR_GENERAL_PROTOCOL_ERROR); >>> + >>> + } else if (c->close) { >>> + ngx_quic_reset_stream(c, NGX_HTTP_V3_ERR_REQUEST_REJECTED); >>> + >>> + } else if (c->requests == 0 || c->error) { >>> + ngx_quic_reset_stream(c, NGX_HTTP_V3_ERR_INTERNAL_ERROR); >>> + } >>> } >> >> The c->requests check looks suspicious, >> is it something to catch a not yet initialized request? > > If for whatever reason we close the stream connection before creating a > request > (thus c->requests == 0), that is a clear signal we need to reset the stream > with NGX_HTTP_V3_ERR_INTERNAL_ERROR error code. The exceptions are > timeout and connection reuse, but these cases are handled just before. > All other cases are errors (failed allocation, recv() failure etc). > >>> static void >>> ngx_http_v3_cleanup_request(void *data) >>> { >>> - ngx_connection_t *c = data; >>> + ngx_http_request_t *r = data; >>> >>> + ngx_connection_t *c; >>> ngx_http_v3_session_t *h3c; >>> ngx_http_core_loc_conf_t *clcf; >>> >>> + c = r->connection; >>> + >>> + if (!r->response_sent) { >>> + c->error = 1; >>> + } >>> + >>> h3c = ngx_http_v3_get_session(c); >>> >>> if (--h3c->nrequests == 0) { >>> diff --git a/src/http/v3/ngx_http_v3_streams.c >>> b/src/http/v3/ngx_http_v3_streams.c >>> --- a/src/http/v3/ngx_http_v3_streams.c >>> +++ b/src/http/v3/ngx_http_v3_streams.c >>> @@ -49,7 +49,8 @@ ngx_http_v3_init_uni_stream(ngx_connecti >>> ngx_http_v3_finalize_connection(c, >>> NGX_HTTP_V3_ERR_STREAM_CREATION_ERROR, >>> "reached maximum number of uni >>> streams"); >>> - ngx_http_close_connection(c); >>> + c->data = NULL; >>> + ngx_http_v3_close_uni_stream(c); >>> return; >>> } >>> >>> @@ -57,7 +58,11 @@ ngx_http_v3_init_uni_stream(ngx_connecti >>> >>> us = ngx_pcalloc(c->pool, sizeof(ngx_http_v3_uni_stream_t)); >>> if (us == NULL) { >>> - ngx_http_close_connection(c); >>> + ngx_http_v3_finalize_connection(c, >>> + NGX_HTTP_V3_ERR_INTERNAL_ERROR, >>> + "memory allocation error"); >>> + c->data = NULL; >>> + ngx_http_v3_close_uni_stream(c); >>> return; >>> } >>> >>> @@ -79,12 +84,12 @@ ngx_http_v3_close_uni_stream(ngx_connect >>> ngx_http_v3_session_t *h3c; >>> ngx_http_v3_uni_stream_t *us; >>> >>> - us = c->data; >>> - h3c = ngx_http_v3_get_session(c); >>> - >>> ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "http3 close stream"); >>> >>> - if (us->index >= 0) { >>> + us = c->data; >>> + >>> + if (us && us->index >= 0) { >>> + h3c = ngx_http_v3_get_session(c); >>> h3c->known_streams[us->index] = NULL; >>> } >>> >> >> Is there any difference after switching to ngx_http_v3_close_uni_stream(), >> besides ngx_stat_active now no longer decremented? This itself looks like >> a right change, since ngx_stat_active isn't incremented for uni streams. > > ngx_stat_active is indeed a good reason. The other reason is > ngx_http_v3_reset_connection() which is called when closing an HTTP/3 stream > connection. A uni stream connection is also an HTTP/3 stream connection, but > we don't want to call ngx_http_v3_reset_connection() for it. We only > want to call it for request streams. We could add another condition, but it's > easier not to call common http code for uni streams at all.
The point is QUIC connection finalization includes ngx_quic_close_streams(), which prevents further I/O on streams, so stream reset won't have an effect. In that sense, the patch could be simplified. On the other hand, with 3rd patch applied, this is certainly no longer true: calling ngx_http_close_connection() on unidirectional streams won't only mean a unidirectional stream cancellation non-sense, but that would also invoke creating a decoder stream as part of closing the given uni stream in case it doesn't exist yet. That stream cancellation would even be sent after CC, since calling ngx_quic_close_streams() already happened. Still, I don't think it should normally happen such that we would need an additional check in ngx_quic_open_stream() in addition to the below patch. To sum up, your change looks good. > >> BTW, we need additional checks to prevent processing new streams >> after ngx_http_v3_finalize_connection(). > > Makes sense. > >> # HG changeset patch >> # User Sergey Kandaurov <[email protected]> >> # Date 1636549164 -10800 >> # Wed Nov 10 15:59:24 2021 +0300 >> # Branch quic >> # Node ID 924ee879f8befa1ec574d41a3979e5c5c5db8639 >> # Parent c6386afdd1552105c18ce5c47b4b5cd6f6de8b88 >> QUIC: stop processing new client streams on quic connection error. >> >> 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 >> @@ -314,7 +314,7 @@ ngx_quic_create_client_stream(ngx_connec >> >> qc = ngx_quic_get_connection(c); >> >> - if (qc->shutdown) { >> + if (qc->shutdown || qc->error) { > > Why not qc->closing? Yes, indeed. > >> return NGX_QUIC_STREAM_GONE; >> } >> >> @@ -385,7 +385,7 @@ ngx_quic_create_client_stream(ngx_connec >> return NULL; >> } >> >> - if (qc->shutdown) { >> + if (qc->shutdown || qc->error) { >> return NGX_QUIC_STREAM_GONE; >> } >> } >> -- Sergey Kandaurov _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
