Hello!

On Thu, Feb 02, 2023 at 01:05:19PM +0400, Sergey Kandaurov wrote:

> > On 29 Jan 2023, at 00:03, Maxim Dounin <mdounin@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@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@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@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.

Thanks for the review, pushed to http://mdounin.ru/hg/nginx.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel