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-le...@nginx.org
