On Mon, Dec 13, 2021 at 03:03:58PM +0300, Roman Arutyunyan wrote: > On Fri, Dec 10, 2021 at 10:38:00AM +0300, Vladimir Homutov wrote: > > 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 <a...@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) > > OK let's merge them. > > > > [..] > > > > > > -- > > > Roman Arutyunyan > > > > > # HG changeset patch > > > # User Roman Arutyunyan <a...@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 <a...@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. > > OK, we can do this. I think these checks will be changed in the future anyway > when we introduce stream lingering. > > > > + 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 > > I suggest that we use rev->error instead, which is even more symmetrical. > Also, I removed setting wev->ready, which makes no sense in the shutdown > function. > > > - it seems perfectly natural for recv shutdown to set some flag > > prevents reading > > rev->pending_eof does not prevent reading. We can still read everything > that's > buffered. Also, I feel like messing with this flag will break its semantics, > so I'd rather leave it alone and use rev->error. > > I think event flag checks will also be removed in the future and replaced > with stream send/recv states as specified by the RFC. This itself is a good > change, but we'll also need this to implement stream lingering. > > > 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. > > The reason for setting rev->pending_eof was to skip updating stream flow > control in ngx_quic_update_flow(). The rev->error flag can do that too. > I removed setting rev->pending_eof from ngx_quic_stream_cleanup_handler(). > > > > (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 > > OK, switched to RDWR. Although, I don't like either name. Since > NGX_WRITE_SHUTDOWN already exists, NGX_READ_SHUTDOWN is an obvious one, but > there's no good name for RDWR. I thought about calling it NGX_FULL_SHUTDOWN. > Anyway, we can leave NGX_RDWR_SHUTDOWN for now and keep in mind a possible > rename in the future.
> # HG changeset patch > # User Roman Arutyunyan <a...@nginx.com> > # Date 1639396182 -10800 > # Mon Dec 13 14:49:42 2021 +0300 > # Branch quic > # Node ID 23880e4ad3e2986c189cf61d8409066f2b31590e > # Parent 0692355a3519024ed9b3a71a7216dcf6fe7e31ca > QUIC: write and full stream shutdown support. > > Full stream shutdown is now called from stream cleanup handler instead of > explicitly sending frames. > > 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,43 @@ ngx_quic_reset_stream(ngx_connection_t * > ngx_int_t > ngx_quic_shutdown_stream(ngx_connection_t *c, int how) > { > + ngx_quic_stream_t *qs; > + > + qs = c->quic; > + > + if (how == NGX_RDWR_SHUTDOWN || how == NGX_WRITE_SHUTDOWN) { > + if ((qs->id & NGX_QUIC_STREAM_SERVER_INITIATED) > + || (qs->id & NGX_QUIC_STREAM_UNIDIRECTIONAL) == 0) > + { > + if (ngx_quic_shutdown_stream_send(c) != NGX_OK) { > + return NGX_ERROR; > + } > + } > + } > + > + if (how == NGX_RDWR_SHUTDOWN || how == NGX_READ_SHUTDOWN) { > + if ((qs->id & NGX_QUIC_STREAM_SERVER_INITIATED) == 0 > + || (qs->id & NGX_QUIC_STREAM_UNIDIRECTIONAL) == 0) > + { > + 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; > - } > - > wev = c->write; > > if (wev->error) { > @@ -283,7 +312,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; > @@ -297,13 +326,56 @@ ngx_quic_shutdown_stream(ngx_connection_ > > ngx_quic_queue_frame(qc, frame); > > - wev->ready = 1; > wev->error = 1; > > return NGX_OK; > } > > > +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; > + > + rev = c->read; > + > + if (rev->pending_eof || rev->error) { > + return NGX_OK; > + } > + > + qs = c->quic; > + 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); > + > + rev->error = 1; > + > + return NGX_OK; > +} > + > + > static ngx_quic_stream_t * > ngx_quic_get_stream(ngx_connection_t *c, uint64_t id) > { > @@ -916,30 +988,10 @@ ngx_quic_stream_cleanup_handler(void *da > goto done; > } > > - c->read->pending_eof = 1; > + (void) ngx_quic_shutdown_stream(c, NGX_RDWR_SHUTDOWN); > > (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) { > @@ -959,37 +1011,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_RDWR_SHUTDOWN SHUT_RDWR > > typedef int ngx_socket_t; looks good! _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel