Hello!

On Fri, Nov 04, 2022 at 09:00:33AM +0000, Ciel via nginx-devel wrote:

> Hi Maxim,
>
> Thanks for the quick reply.
>
> > It looks like an attempt to fix ticket #1263
> > (https://trac.nginx.org/nginx/ticket/1263). I've linked this
> > thread to the ticket. It might be a good idea to add a reference
> > into commit log.
>
> I encountered this problem building my own website, and spent some hours to
> figure out the problem and patch. I didn't guess this bug have such history.
> Reference will be in the updated patch.
>
> > In many cases the main request context is not needed. It might be
> > a good idea to create it only when needed, and avoid doing so if
> > it isn't.
> >
> > Further, in theory, SSI processing of a (in-memory/background)
> > subrequest might happen even before main request is actually seen
> > by the SSI module, so the actual context will be created later.
> > This probably needs to be taken into account.
>
> As we only use the main context for variables and stub blocks, it seems appreciable
> to have the context created on demand. However, we can't say if the SSI templates
> use these commands until we processed them. So I think this left us merely two
> options:
>
> a. create main context for every requests having SSI enabled (previous patch)
> b. check main context existence everywhere we use it
>
> From my purview, most SSI templates have some variables involved, so just go ahead
> and create context can save many checks, while not introducing much waste of memory.

Certainly there are SSI templates which only use include commands,
but you are probably right and creating the main request context
would be easier. With on-demand creation, even if we'll introduce
a function to obtain/create main request context, we'll have to
check for memory allocation errors in all places we use it.

Another option might be to link the first subrequest's context as
the main one - till the main request context is created (if at
all). Especially given that we anyway have to upgrade the main
request context if the main request is seen after the first
subrequest. This will imply an additional check in the body
filter along with the flag, but a trivial one.

> > This does not look like a good solution to filter out processing
> > of a main request if SSI is not enabled for it. Rather, there
> > should be an explicit flag, so it would be possible to avoid
> > evaluation of complex constructs on each body filter call - there
> > can be a lot of such calls.
>
> Totally agree, and this can be checked by `ngx_http_ssi_header_filter` in main request,
> to initialize the dummy context if needed. But I wonder shall I add a bit field into
> `ngx_http_ssi_ctx_t` (as there're already some so no memory cost) or add an
> `ngx_flag_t` (8B on amd64) instead.

The ngx_flag_t type is configuration-specific, and shouldn't be
used in runtime structures such as SSI contexts.

The general rule for runtime flags is that they are created as
ngx_uint_t with appropriate comment if there are no other
bit fields in the structure, for example
(src/http/modules/ngx_http_headers_filter_module.c):

ngx_uint_t always; /* unsigned always:1 */

and upgraded to proper bit fields as long as there is more than
one.

Given that ngx_http_ssi_ctx_t already contains bit fields, the new
flag should be added as a bit field.

> In the issue tracker 6 years ago, you mentioned,
>
> > It was written when there were no subrequests except subrequests created by the SSI
> > module itself, and assumes in many places that its context always exists in the main
> > request.
>
> Though I'm not 100% familiar with the process of subrequests, this arise some more
> questions in my mind: If subrequests are processed by the SSI filter (before postpone)
> asynchronously and concurrently, could this introduce some out-of-order execution of
> SSI commands among subrequests, then lead to non-determined result? If this really
> happens, should we move the SSI filter past the postpone filter? Looking forward to
> some enlightenment.

The idea of the postpone filter is to glue all subrequests
together in the correct order. Basically, it makes it possible to
execute SSI subrequests in parallel.

For sure this can be a problem if subrequests are used without
taking this into account. For example, if you'll set a variable
in an SSI include virtual, trying to use it in the main SSI
document or other included files might easily fail because it is
not yet set.

To fix this, there is a special parameter of the include SSI
command, "wait". Quoting docs
(https://nginx.org/en/docs/http/ngx_http_ssi_module.html#commands):

: wait
:
: a non-standard parameter that instructs to wait for a request to
: fully complete before continuing with SSI processing, for example:
:
: <!--# include virtual="/remote/body.php?argument=value" wait="yes" -->

This makes it possible to add "sequence points" to SSI, resolving
undefined behaviour due to parallel execution of requests.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org