Opened 4 months ago

Closed 4 months ago

#2215 closed defect (wontfix)

Maybe a minior bug in $request_completion code

Reported by: chronolaw@… Owned by:
Priority: minor Milestone: nginx-1.21
Component: documentation Version: 1.19.x
Keywords: variable Cc:
uname -a: Linux
nginx -V: nginx 1.19.10

Description

Hi,

When I do some test on NGINX variables, I found some strange behavior about $request_completion.

Give the config:

log_format resp_demo '$request_completion';

location /resp {

access_log /var/log/nginx/access.log resp_demo;
return 200 '$request_completion\n';

}

Then I run curl 127.1/resp, we get (empty string).
Because $request_completion is defined as CACHEABLE, so when we log it in access.log ,it will be the same empty value, not the right value 'OK'.

I think $request_completion should act as other variables like $is_arg, Shall we change the flag to NGX_HTTP_VAR_NOCACHEABLE?

Here is my patch:

--- ngx_http_variables.c.old 2021-07-12 20:11:59.985843678 +0800
+++ ngx_http_variables.c 2021-07-12 20:12:22.881285689 +0800
@@ -282,7 +282,7 @@

{ ngx_string("request_completion"), NULL,

ngx_http_variable_request_completion,

  • 0, 0, 0 },

+ 0, NGX_HTTP_VAR_NOCACHEABLE, 0 },

{ ngx_string("request_body"), NULL,

ngx_http_variable_request_body,

Attachments (1)

request_completion.patch (381 bytes ) - added by chronolaw@… 4 months ago.

Download all attachments as: .zip

Change History (5)

by chronolaw@…, 4 months ago

Attachment: request_completion.patch added

comment:1 by Maxim Dounin, 4 months ago

This is more or less a general problem: there are a lot of variables which can be in certain cases accessed before they have a known meaningful value, and if accessed before they are usable the wrong value can be cached. On the other hand, the value does not change once it is properly set. Examples include $sent_http_* variables before the response is generated, $remote_addr (if updated by the realip module, see #603), $request_body before the request body is read, and many others.

The $request_completion variable is one of such variables. It doesn't make sense to use it anywhere but during logging, though an attempt to anyway use it, as in your configuration snippet, will result in the wrong value being cached. While it is possible to fix this by making the variable non-cacheable, it is probably not the best possible solution, as this makes proper use of the variable less effective. A better solution might be to avoid incorrect use of the variable.

If you think that using $request_completion make sense before the logging phase, please elaborate.

comment:2 by chronolaw@…, 4 months ago

OK, I got it.But I have another question:

What the proper meaning of $request_completion. It is only useful in log phase, but at this time the request is done, what can I do with $request_completion?

comment:3 by chronolaw@…, 4 months ago

Or maybe the empty value of $request_completion is the right meaning of it.

In the phases before log, we get empty value, this will tell us we are still processing request, then in the log phase, the request is definitely completed, so we should not care about $request_completion.

comment:4 by Maxim Dounin, 4 months ago

Resolution: wontfix
Status: newclosed

The $request_completion variable is documented as follows:

$request_completion
“OK” if a request has completed, or an empty string otherwise

The "request has completed" means that all the response data has been sent to the client socket. That is, it is always empty before the logging phase (and hence it does not make sense to use it anywhere before logging). At the logging phase, it can be either "OK" (if all the response data has been successfully sent) or an empty string (if an error occurred during sending, for example, if the client closed connection).

Closing this as "wontfix" as it looks like there are no significant reasons to make the variable non-cacheable. As outlined above, a better solution is to avoid using the variable before the logging phase where the variable make sense.

Note: See TracTickets for help on using tickets.