[PATCH] HTTP/2: made it possible to flush response headers (ticket #1743)

[Sergey Kandaurov]

> On 27 Jan 2022, at 04:24, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1643225036 -10800
> #      Wed Jan 26 22:23:56 2022 +0300
> # Node ID f76e0cf8e525996563e5f0092fa48a4fee873e93
> # Parent  56ead48cfe885e8b89b30017459bf621b21d95f5
> HTTP/2: made it possible to flush response headers (ticket #1743).
> 
> Response headers can be buffered in the SSL buffer.  But stream's fake
> connection buffered flag did not reflect this, so any attempts to flush
> the buffer without sending additional data were stopped by the write filter.
> 
> It does not seem to be possible to reflect this in fc->buffered though, as
> we never known if main connection's c->buffered corresponds to the particular
> stream or not.  As such, fc->buffered might prevent request finalization
> due to sending data on some other stream.
> 
> Fix is to implement handling of flush buffers when c->need_last_buf is set,
> similarly to the existing last buffer handling.
> 
> diff --git a/src/http/ngx_http_write_filter_module.c b/src/http/ngx_http_write_filter_module.c
> --- a/src/http/ngx_http_write_filter_module.c
> +++ b/src/http/ngx_http_write_filter_module.c
> @@ -227,7 +227,7 @@ ngx_http_write_filter(ngx_http_request_t
> 
>     if (size == 0
>         && !(c->buffered & NGX_LOWLEVEL_BUFFERED)
> -        && !(last && c->need_last_buf))
> +        && !((last || flush) && c->need_last_buf))
>     {
>         if (last || flush || sync) {
>             for (cl = r->out; cl; /* void */) {
> diff --git a/src/http/v2/ngx_http_v2_filter_module.c b/src/http/v2/ngx_http_v2_filter_module.c
> --- a/src/http/v2/ngx_http_v2_filter_module.c
> +++ b/src/http/v2/ngx_http_v2_filter_module.c
> @@ -1815,7 +1815,11 @@ ngx_http_v2_waiting_queue(ngx_http_v2_co
> static ngx_inline ngx_int_t
> ngx_http_v2_filter_send(ngx_connection_t *fc, ngx_http_v2_stream_t *stream)
> {
> -    if (stream->queued == 0) {
> +    ngx_connection_t  *c;
> +
> +    c = stream->connection->connection;
> +
> +    if (stream->queued == 0 && !c->buffered) {
>         fc->buffered &= ~NGX_HTTP_V2_BUFFERED;
>         return NGX_OK;
>     }
> 

Nitpicking:
Shouldn't NGX_HTTP_V2_BUFFERED still be cleared regardless?

    if (stream->queued == 0) {
        fc->buffered &= ~NGX_HTTP_V2_BUFFERED;

        if (!c->buffered) {
            return NGX_OK;
        }
    }

It doesn't seem to change anything, at a glance.
However, this would save a contract that
(fc->buffered & NGX_HTTP_V2_BUFFERED) is influenced by stream->queued.

Otherwise, looks good.

-- 
Sergey Kandaurov