[PATCH] QUIC: stream lingering

[Vladimir Homutov]

On Tue, Feb 01, 2022 at 04:39:59PM +0300, Roman Arutyunyan wrote:
> # HG changeset patch
> # User Roman Arutyunyan <arut at nginx.com>
> # 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.