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 <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)
OK let's merge them.
> > [..]
> >
> > --
> > 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.
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.
> > 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
--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <arut@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;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
