[PATCH] Gzip static: ranges support (ticket #2349)
Sergey Kandaurov
pluknet at nginx.com
Thu Feb 2 09:05:19 UTC 2023
> On 29 Jan 2023, at 00:03, Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> Hello!
>
> On Wed, Jan 25, 2023 at 01:50:56PM +0400, Sergey Kandaurov wrote:
>
>>> On 24 Jan 2023, at 06:19, Maxim Dounin <mdounin at mdounin.ru> wrote:
>>>
>>>>> [..]
>>>
>>> diff --git a/src/http/modules/ngx_http_flv_module.c b/src/http/modules/ngx_http_flv_module.c
>>> --- a/src/http/modules/ngx_http_flv_module.c
>>> +++ b/src/http/modules/ngx_http_flv_module.c
>>> @@ -235,6 +235,7 @@ ngx_http_flv_handler(ngx_http_request_t
>>> b->in_file = b->file_last ? 1: 0;
>>
>> btw, there are old style issues originated in the static module
>> (it was fixed in gzip_static when it was copied from there)
>
> This is actually an (old) style, which was previously used in many
> places. Now it remains mostly in multi-line forms of the
> conditional operator, e.g.:
>
> r->loc_conf = (node->exact) ? node->exact->loc_conf:
> node->inclusive->loc_conf;
>
> Single-line form is indeed mostly unused now, and probably it's
> time to remove remaining occurrences. Added a patch for this.
>
> [...]
>
>>> @@ -238,10 +238,6 @@ ngx_http_static_handler(ngx_http_request
>>> return NGX_HTTP_INTERNAL_SERVER_ERROR;
>>> }
>>>
>>> - if (r != r->main && of.size == 0) {
>>> - return ngx_http_send_header(r);
>>> - }
>>> -
>>> r->allow_ranges = 1;
>>>
>>> /* we need to allocate all before the header would be sent */
>>> @@ -268,6 +264,7 @@ ngx_http_static_handler(ngx_http_request
>>> b->in_file = b->file_last ? 1: 0;
>>> b->last_buf = (r == r->main) ? 1: 0;
>>> b->last_in_chain = 1;
>>> + b->sync = (r == r->main || b->file_last) ? 0 : 1;
>>>
>>> b->file->fd = of.fd;
>>> b->file->name = path;
>>>
>>
>> In addition to the static module change, any reason not to include
>> ngx_http_cache_send()? It has a similar behaviour to skip cached
>> empty body if served in subrequests. Yet another similar place is
>> subrequests processed with rewrite such as "return 204;".
>
> Indeed, thanks for catching, missed these two.
>
> Also, looking into this places I tend to think it's better to use
>
> b->sync = (b->last_buf || b->in_file) ? 0 : 1;
>
> instead, which is going to be simpler and more universal across
> all uses.
I agree.
>
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1674872423 -10800
> # Sat Jan 28 05:20:23 2023 +0300
> # Node ID 1c3b78d7cdc9e7059e90aa1670d58fd37927a1a2
> # Parent 106328a70f4ecb32f828d33e5cd66c861e455f92
> Style.
>
> diff --git a/src/event/ngx_event_connectex.c b/src/event/ngx_event_connectex.c
> --- a/src/event/ngx_event_connectex.c
> +++ b/src/event/ngx_event_connectex.c
> @@ -127,8 +127,8 @@ void ngx_iocp_wait_events(int main)
> conn[0] = NULL;
>
> for ( ;; ) {
> - offset = (nevents == WSA_MAXIMUM_WAIT_EVENTS + 1) ? 1: 0;
> - timeout = (nevents == 1 && !first) ? 60000: INFINITE;
> + offset = (nevents == WSA_MAXIMUM_WAIT_EVENTS + 1) ? 1 : 0;
> + timeout = (nevents == 1 && !first) ? 60000 : INFINITE;
>
> n = WSAWaitForMultipleEvents(nevents - offset, events[offset],
> 0, timeout, 0);
> diff --git a/src/http/modules/ngx_http_flv_module.c b/src/http/modules/ngx_http_flv_module.c
> --- a/src/http/modules/ngx_http_flv_module.c
> +++ b/src/http/modules/ngx_http_flv_module.c
> @@ -232,7 +232,7 @@ ngx_http_flv_handler(ngx_http_request_t
> b->file_pos = start;
> b->file_last = of.size;
>
> - b->in_file = b->file_last ? 1: 0;
> + b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
>
> diff --git a/src/http/modules/ngx_http_static_module.c b/src/http/modules/ngx_http_static_module.c
> --- a/src/http/modules/ngx_http_static_module.c
> +++ b/src/http/modules/ngx_http_static_module.c
> @@ -265,8 +265,8 @@ ngx_http_static_handler(ngx_http_request
> b->file_pos = 0;
> b->file_last = of.size;
>
> - b->in_file = b->file_last ? 1: 0;
> - b->last_buf = (r == r->main) ? 1: 0;
> + b->in_file = b->file_last ? 1 : 0;
> + b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
>
> b->file->fd = of.fd;
> diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c
> +++ b/src/http/ngx_http_file_cache.c
> @@ -1600,8 +1600,8 @@ ngx_http_cache_send(ngx_http_request_t *
> b->file_pos = c->body_start;
> b->file_last = c->length;
>
> - b->in_file = (c->length - c->body_start) ? 1: 0;
> - b->last_buf = (r == r->main) ? 1: 0;
> + b->in_file = (c->length - c->body_start) ? 1 : 0;
> + b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
>
> b->file->fd = c->file.fd;
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1674872613 -10800
> # Sat Jan 28 05:23:33 2023 +0300
> # Node ID f5515e727656b2cc529b26a0fe6b4496c62e32b2
> # Parent 1c3b78d7cdc9e7059e90aa1670d58fd37927a1a2
> Fixed "zero size buf" alerts with subrequests.
>
> Since 4611:2b6cb7528409 responses from the gzip static, flv, and mp4 modules
> can be used with subrequests, though empty files were not properly handled.
> Empty gzipped, flv, and mp4 files thus resulted in "zero size buf in output"
> alerts. While valid corresponding files are not expected to be empty, such
> files shouldn't result in alerts.
>
> Fix is to set b->sync on such empty subrequest responses, similarly to what
> ngx_http_send_special() does.
>
> Additionally, the static module, the ngx_http_send_response() function, and
> file cache are modified to do the same instead of not sending the response
> body at all in such cases, since not sending the response body at all is
> believed to be at least questionable, and might break various filters
> which do not expect such behaviour.
>
> diff --git a/src/http/modules/ngx_http_flv_module.c b/src/http/modules/ngx_http_flv_module.c
> --- a/src/http/modules/ngx_http_flv_module.c
> +++ b/src/http/modules/ngx_http_flv_module.c
> @@ -235,6 +235,7 @@ ngx_http_flv_handler(ngx_http_request_t
> b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (b->last_buf || b->in_file) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
> diff --git a/src/http/modules/ngx_http_gzip_static_module.c b/src/http/modules/ngx_http_gzip_static_module.c
> --- a/src/http/modules/ngx_http_gzip_static_module.c
> +++ b/src/http/modules/ngx_http_gzip_static_module.c
> @@ -273,6 +273,7 @@ ngx_http_gzip_static_handler(ngx_http_re
> b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (b->last_buf || b->in_file) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
> diff --git a/src/http/modules/ngx_http_mp4_module.c b/src/http/modules/ngx_http_mp4_module.c
> --- a/src/http/modules/ngx_http_mp4_module.c
> +++ b/src/http/modules/ngx_http_mp4_module.c
> @@ -714,6 +714,7 @@ ngx_http_mp4_handler(ngx_http_request_t
> b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (b->last_buf || b->in_file) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
> diff --git a/src/http/modules/ngx_http_static_module.c b/src/http/modules/ngx_http_static_module.c
> --- a/src/http/modules/ngx_http_static_module.c
> +++ b/src/http/modules/ngx_http_static_module.c
> @@ -238,10 +238,6 @@ ngx_http_static_handler(ngx_http_request
> return NGX_HTTP_INTERNAL_SERVER_ERROR;
> }
>
> - if (r != r->main && of.size == 0) {
> - return ngx_http_send_header(r);
> - }
> -
> r->allow_ranges = 1;
>
> /* we need to allocate all before the header would be sent */
> @@ -268,6 +264,7 @@ ngx_http_static_handler(ngx_http_request
> b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (b->last_buf || b->in_file) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
> diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
> --- a/src/http/ngx_http_core_module.c
> +++ b/src/http/ngx_http_core_module.c
> @@ -1803,10 +1803,6 @@ ngx_http_send_response(ngx_http_request_
> }
> }
>
> - if (r != r->main && val.len == 0) {
> - return ngx_http_send_header(r);
> - }
> -
> b = ngx_calloc_buf(r->pool);
> if (b == NULL) {
> return NGX_HTTP_INTERNAL_SERVER_ERROR;
> @@ -1817,6 +1813,7 @@ ngx_http_send_response(ngx_http_request_
> b->memory = val.len ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (b->last_buf || b->memory) ? 0 : 1;
>
> out.buf = b;
> out.next = NULL;
> diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c
> +++ b/src/http/ngx_http_file_cache.c
> @@ -1575,10 +1575,6 @@ ngx_http_cache_send(ngx_http_request_t *
> ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> "http file cache send: %s", c->file.name.data);
>
> - if (r != r->main && c->length - c->body_start == 0) {
> - return ngx_http_send_header(r);
> - }
> -
> /* we need to allocate all before the header would be sent */
>
> b = ngx_calloc_buf(r->pool);
> @@ -1603,6 +1599,7 @@ ngx_http_cache_send(ngx_http_request_t *
> b->in_file = (c->length - c->body_start) ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (b->last_buf || b->in_file) ? 0 : 1;
>
> b->file->fd = c->file.fd;
> b->file->name = c->file.name;
>
Both changes look good to me, please commit.
--
Sergey Kandaurov
More information about the nginx-devel
mailing list