On Fri, Nov 26, 2021 at 04:11:33PM +0300, Roman Arutyunyan wrote:
> On Thu, Nov 25, 2021 at 05:20:51PM +0300, Roman Arutyunyan wrote:
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1637695967 -10800
> > # Tue Nov 23 22:32:47 2021 +0300
> > # Branch quic
> > # Node ID e1de02d829f7f85b1e2e6b289ec4c20318712321
> > # Parent 3d2354bfa1a2a257b9f73772ad0836585be85a6c
> > QUIC: stream recv shutdown support.
> >
> > Recv shutdown sends STOP_SENDING to client. Both send and recv shutdown
> > functions are now called from stream cleanup handler. While here, setting
> > c->read->pending_eof is moved down to fix recv shutdown in the cleanup handler.
>
> This definitely needs some improvement. Now it's two patches.

I suggest merging both into one (also, second needs rebasing)

>
> [..]
>
> --
> Roman Arutyunyan

> # HG changeset patch
> # User Roman Arutyunyan <arut@nginx.com>
> # Date 1637931593 -10800
> # Fri Nov 26 15:59:53 2021 +0300
> # Branch quic
> # Node ID c2fa3e7689a4e286f45ccbac2288ade5966273b8
> # Parent 3d2354bfa1a2a257b9f73772ad0836585be85a6c
> QUIC: do not shutdown write part of a client uni stream.
>
> 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
> @@ -267,13 +267,20 @@ ngx_quic_shutdown_stream(ngx_connection_
> return NGX_OK;
> }
>
> + qs = c->quic;
> +
> + if ((qs->id & NGX_QUIC_STREAM_SERVER_INITIATED) == 0
> + && (qs->id & NGX_QUIC_STREAM_UNIDIRECTIONAL))
> + {
> + return NGX_OK;
> + }
> +
> wev = c->write;
>
> if (wev->error) {
> return NGX_OK;
> }
>
> - qs = c->quic;
> pc = qs->parent;
> qc = ngx_quic_get_connection(pc);
>

this one looks good


> # HG changeset patch
> # User Roman Arutyunyan <arut@nginx.com>
> # Date 1637932014 -10800
> # Fri Nov 26 16:06:54 2021 +0300
> # Branch quic
> # Node ID ed0cefd9fc434a7593f2f9e4b9a98ce65aaf05e9
> # Parent c2fa3e7689a4e286f45ccbac2288ade5966273b8
> QUIC: write and full stream shutdown support.
>
> Full stream shutdown is now called from stream cleanup handler instead of
> explicitly sending frames. The call is moved up not to be influenced by
> setting c->read->pending_eof, which was erroneously set too early.
>
> 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
> @@ -13,6 +13,8 @@
> #define NGX_QUIC_STREAM_GONE (void *) -1
>
>
> +static ngx_int_t ngx_quic_shutdown_stream_send(ngx_connection_t *c);
> +static ngx_int_t ngx_quic_shutdown_stream_recv(ngx_connection_t *c);
> static ngx_quic_stream_t *ngx_quic_get_stream(ngx_connection_t *c, uint64_t id);
> static ngx_int_t ngx_quic_reject_stream(ngx_connection_t *c, uint64_t id);
> static void ngx_quic_init_stream_handler(ngx_event_t *ev);
> @@ -257,16 +259,31 @@ ngx_quic_reset_stream(ngx_connection_t *
> ngx_int_t
> ngx_quic_shutdown_stream(ngx_connection_t *c, int how)
> {
> + if (how == NGX_RW_SHUTDOWN || how == NGX_WRITE_SHUTDOWN) {
> + if (ngx_quic_shutdown_stream_send(c) != NGX_OK) {
> + return NGX_ERROR;
> + }
> + }
> +
> + if (how == NGX_RW_SHUTDOWN || how == NGX_READ_SHUTDOWN) {
> + if (ngx_quic_shutdown_stream_recv(c) != NGX_OK) {
> + return NGX_ERROR;
> + }
> + }
> +
> + return NGX_OK;
> +}
> +
> +
> +static ngx_int_t
> +ngx_quic_shutdown_stream_send(ngx_connection_t *c)
> +{
> ngx_event_t *wev;
> ngx_connection_t *pc;
> ngx_quic_frame_t *frame;
> ngx_quic_stream_t *qs;
> ngx_quic_connection_t *qc;
>
> - if (how != NGX_WRITE_SHUTDOWN) {
> - return NGX_OK;
> - }
> -
> qs = c->quic;
>
> if ((qs->id & NGX_QUIC_STREAM_SERVER_INITIATED) == 0
> @@ -290,7 +307,7 @@ ngx_quic_shutdown_stream(ngx_connection_
> }
>
> ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0,
> - "quic stream id:0x%xL shutdown", qs->id);
> + "quic stream id:0x%xL send shutdown", qs->id);
>
> frame->level = ssl_encryption_application;
> frame->type = NGX_QUIC_FT_STREAM;
> @@ -311,6 +328,55 @@ ngx_quic_shutdown_stream(ngx_connection_
> }
>
>
> +static ngx_int_t
> +ngx_quic_shutdown_stream_recv(ngx_connection_t *c)
> +{
> + ngx_event_t *rev;
> + ngx_connection_t *pc;
> + ngx_quic_frame_t *frame;
> + ngx_quic_stream_t *qs;
> + ngx_quic_connection_t *qc;
> +
> + qs = c->quic;
> +
> + if ((qs->id & NGX_QUIC_STREAM_SERVER_INITIATED)
> + && (qs->id & NGX_QUIC_STREAM_UNIDIRECTIONAL))
> + {

maybe it's worth trying to move server/client bidi/uni tests into
ngx_quic_shutdown_stream() ? It looks like more natural place to
test which end to shut, and whether we need to do it at all.

> + return NGX_OK;
> + }
> +
> + rev = c->read;
> +
> + if (rev->pending_eof || rev->error) {
> + return NGX_OK;
> + }
> +
> + pc = qs->parent;
> + qc = ngx_quic_get_connection(pc);
> +
> + if (qc->conf->stream_close_code == 0) {
> + return NGX_OK;
> + }
> +
> + frame = ngx_quic_alloc_frame(pc);
> + if (frame == NULL) {
> + return NGX_ERROR;
> + }
> +
> + ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0,
> + "quic stream id:0x%xL recv shutdown", qs->id);
> +
> + frame->level = ssl_encryption_application;
> + frame->type = NGX_QUIC_FT_STOP_SENDING;
> + frame->u.stop_sending.id = qs->id;
> + frame->u.stop_sending.error_code = qc->conf->stream_close_code;
> +
> + ngx_quic_queue_frame(qc, frame);
> +
> + return NGX_OK;
> +}
> +
> +
> static ngx_quic_stream_t *
> ngx_quic_get_stream(ngx_connection_t *c, uint64_t id)
> {
> @@ -925,30 +991,12 @@ ngx_quic_stream_cleanup_handler(void *da
> goto done;
> }
>
> + (void) ngx_quic_shutdown_stream(c, NGX_RW_SHUTDOWN);
> +
> c->read->pending_eof = 1;

I think we need to move setting pending_eof this into
ngx_quic_shutdown_stream_recv():

- the shutdown is supposed to be called only once
- this is the flag tested for this purpose
- this will be symmetrical with send that sets wev->error and tests it
on enter
- it seems perfectly natural for recv shutdown to set some flag
prevents reading

or probalby this need to be done in ngx_quic_shutdown_stream(), as
currently it is set without taking into account uni/bidi server/client
difference.

>
> (void) ngx_quic_update_flow(c, qs->recv_last);
>
> - if ((qs->id & NGX_QUIC_STREAM_SERVER_INITIATED) == 0
> - || (qs->id & NGX_QUIC_STREAM_UNIDIRECTIONAL) == 0)
> - {
> - if (!c->read->pending_eof && !c->read->error
> - && qc->conf->stream_close_code)
> - {
> - frame = ngx_quic_alloc_frame(pc);
> - if (frame == NULL) {
> - goto done;
> - }
> -
> - frame->level = ssl_encryption_application;
> - frame->type = NGX_QUIC_FT_STOP_SENDING;
> - frame->u.stop_sending.id = qs->id;
> - frame->u.stop_sending.error_code = qc->conf->stream_close_code;
> -
> - ngx_quic_queue_frame(qc, frame);
> - }
> - }
> -
> if ((qs->id & NGX_QUIC_STREAM_SERVER_INITIATED) == 0) {
> frame = ngx_quic_alloc_frame(pc);
> if (frame == NULL) {
> @@ -968,37 +1016,8 @@ ngx_quic_stream_cleanup_handler(void *da
> }
>
> ngx_quic_queue_frame(qc, frame);
> -
> - if (qs->id & NGX_QUIC_STREAM_UNIDIRECTIONAL) {
> - /* do not send fin for client unidirectional streams */
> - goto done;
> - }
> }
>
> - if (c->write->error) {
> - goto done;
> - }
> -
> - ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0,
> - "quic stream id:0x%xL send fin", qs->id);
> -
> - frame = ngx_quic_alloc_frame(pc);
> - if (frame == NULL) {
> - goto done;
> - }
> -
> - frame->level = ssl_encryption_application;
> - frame->type = NGX_QUIC_FT_STREAM;
> - frame->u.stream.off = 1;
> - frame->u.stream.len = 1;
> - frame->u.stream.fin = 1;
> -
> - frame->u.stream.stream_id = qs->id;
> - frame->u.stream.offset = c->sent;
> - frame->u.stream.length = 0;
> -
> - ngx_quic_queue_frame(qc, frame);
> -
> done:
>
> (void) ngx_quic_output(pc);
> diff --git a/src/os/unix/ngx_socket.h b/src/os/unix/ngx_socket.h
> --- a/src/os/unix/ngx_socket.h
> +++ b/src/os/unix/ngx_socket.h
> @@ -13,6 +13,8 @@
>
>
> #define NGX_WRITE_SHUTDOWN SHUT_WR
> +#define NGX_READ_SHUTDOWN SHUT_RD
> +#define NGX_RW_SHUTDOWN SHUT_RDWR

I suggest renaming this to NGX_RDWR_SHUTDOWN - 'RW' looks to much like
'WR' and makes confusion

>
> typedef int ngx_socket_t;
>

> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel