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


_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org