On Tue, Feb 01, 2022 at 04:39:59PM +0300, Roman Arutyunyan wrote:
> # HG changeset patch
> # User Roman Arutyunyan
> # Date 1643722727 -10800
> # Tue Feb 01 16:38:47 2022 +0300
> # Branch quic
> # Node ID db31ae16c1f2050be9c9f6b1f117ab6725b97dd4
> # Parent 308ac307b3e6952ef0c5ccf10cc82904c59fa4c3
> QUIC: stream lingering.
>
> Now ngx_quic_stream_t is decoupled from ngx_connection_t in a way that it
> can persist after connection is closed by application. During this period,
> server is expecting stream final size from client for correct flow control.
> Also, buffered output is sent to client as more flow control credit is
> granted.
>
[..]
> +static ngx_int_t
> +ngx_quic_stream_flush(ngx_quic_stream_t *qs)
> +{
> +size_t limit, len;
> +ngx_uint_t last;
> +ngx_chain_t*out, *cl;
> +ngx_quic_frame_t *frame;
> +ngx_connection_t *pc;
> +ngx_quic_connection_t *qc;
> +
> +if (qs->send_state != NGX_QUIC_STREAM_SEND_SEND) {
> +return NGX_OK;
> +}
> +
> +pc = qs->parent;
> +qc = ngx_quic_get_connection(pc);
> +
> +limit = ngx_quic_max_stream_flow(qs);
> +last = 0;
> +
> +out = ngx_quic_read_chain(pc, &qs->out, limit);
> +if (out == NGX_CHAIN_ERROR) {
> +return NGX_ERROR;
> +}
> +
> +len = 0;
> +last = 0;
this assignment looks duplicate.
[..]
> +static ngx_int_t
> +ngx_quic_close_stream(ngx_quic_stream_t *qs)
> +{
> ngx_connection_t *pc;
> ngx_quic_frame_t *frame;
> -ngx_quic_stream_t *qs;
> ngx_quic_connection_t *qc;
>
> -qs = c->quic;
> pc = qs->parent;
> qc = ngx_quic_get_connection(pc);
>
> -ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0,
> - "quic stream id:0x%xL cleanup", qs->id);
> +if (!qc->closing) {
> +if (qs->recv_state == NGX_QUIC_STREAM_RECV_RECV
> +|| qs->send_state == NGX_QUIC_STREAM_SEND_READY
> +|| qs->send_state == NGX_QUIC_STREAM_SEND_SEND)
> +{
so basically this are the states where we need to wait for FIN?
and thus avoid closing till we get it.
I would add a comment here.
[..]
> +if (qs->connection == NULL) {
> +return ngx_quic_close_stream(qs);
> +}
> +
> ngx_quic_set_event(qs->connection->write);
this pattern - check connection, close if NULL and set event seem to
repeat. Maybe it's worth to try to put this check/action into
ngx_quic_set_event somehow ? we could instead have
set_read_event/set_write_event maybe.
> +static ngx_int_t
> +ngx_quic_stream_flush(ngx_quic_stream_t *qs)
> +
[..]
> +if (len == 0 && !last) {
> +return NGX_OK;
> +}
> +
> +frame = ngx_quic_alloc_frame(pc);
> +if (frame == NULL) {
> +return NGX_ERROR;
> +}
> +
> +frame = ngx_quic_alloc_frame(pc);
> +if (frame == NULL) {
> +return NGX_ERROR;
> +}
one more dup here.
Overal, it looks good, but the testing revealed another issue: with big
buffer sizes we run into issue of too long chains in ngx_quic_write_chain().
As discussed, this certainly needs optimization - probably adding some
pointer to the end to facilitate appending, or something else.
___
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-le...@nginx.org
