Ngx_upstream do not cleanup file buffers in subrequest

luislavena · March 8, 2011, 9:26pm

Hello,

Is it reasonable to add following check to not clean/discard fd in case
if it’s sub request?

I developed nginx module and when it started to use tmp buffers under
load I had following error:

[alert] 29663#0: *45789 sendfile() failed (9: Bad file descriptor) while
sending request to upstream, client: 10.1.24.14, server: hss, request:
"POST

So that sub-request cleared file before main request reached it.
Probably this bug should not appear if request body was discarded for
sub request. But in case if you need request body for sub-request you
should be sure that sub-request does not erases main request buffers.

– src/http/ngx_http_upstream.c_orig 2011-03-08 08:01:20.000000000
-0800
+++ src/http/ngx_http_upstream.c 2011-03-08 08:01:41.000000000 -0800
@@ -2033,7 +2033,7 @@ ngx_http_upstream_send_response(ngx_http

 u->header_sent = 1;

if (r->request_body && r->request_body->temp_file) {

if (r->request_body && r->request_body->temp_file && r == r->main)
{
ngx_pool_run_cleanup_file(r->pool,
r->request_body->temp_file->file.fd);
r->request_body->temp_file->file.fd = NGX_INVALID_FILE;
}

Posted at Nginx Forum:

magz · March 9, 2011, 12:17pm

Hello!

On Tue, Mar 08, 2011 at 03:24:47PM -0500, magz wrote:

"POST

So that sub-request cleared file before main request reached it.
Probably this bug should not appear if request body was discarded for
sub request. But in case if you need request body for sub-request you
should be sure that sub-request does not erases main request buffers.

In official nginx, subrequests can’t appear before main
request.

On the other hand, main request may clear body as well, and it
won’t be available for subrequests as a result. This problem may
be easily seen with ssi module. The other possible case when this
problem may appear is filter finalization (e.g. as done by image
filter module).

That is, just disabling body cleanup for subrequests is clearly
not enough.

I’ve already posted the patch which removes the code in question
at all (as a reply to some real problems seen with ssi and POSTs,
somewhere in russian mailing list). Probably it should be made
controllable via some configuration directive if there are some
real use cases which may be seriously affected by removing this
code. I would like to hear something from Igor about this.

BTW, just for history: the code in question was introduced in
nginx 0.3.3 with the following log message:

*) Bugfix: a temporary file with client request body now is removed
   just after the response header was transferred to a client.

Maxim D.

magz · March 9, 2011, 7:48pm

Maxim D. Wrote:

So that sub-request cleared file before main
request.

controllable via some configuration directive if
*) Bugfix: a temporary file with client
request body now is removed
just after the response header was
transferred to a client.

Maxim D.

additional flag to ngx_http_subrequest() has to work for this issue.

NGX_INVALID_FILE;

nginx Info Page

nginx mailing list
[email protected]
nginx Info Page

Posted at Nginx Forum:

magz · March 9, 2011, 8:02pm

Hello!

On Wed, Mar 09, 2011 at 01:46:56PM -0500, magz wrote:

clean/discard fd in case

"POST
In official nginx, subrequests can’t appear before
done by image
somewhere in russian mailing list). Probably it
nginx 0.3.3 with the following log message:
*) Bugfix: a temporary file with client
request body now is removed
just after the response header was
transferred to a client.

Maxim D.
additional flag to ngx_http_subrequest() has to work for this issue.

Additional flag to ngx_http_subrequest() can’t help if you aren’t
even going to execute subrequests (as in filter finalization case)
and can’t help with ssi either, as subrequests are executed after
main request already cleared request body.

Maxim D.

magz · March 9, 2011, 8:24pm

Something like that. So we can do:
rc = ngx_http_subrequest(r, &subreq_loc , &args,
&sr, psr, NGX_HTTP_REQUEST_KEEP_TEMP_FILE);

cat ngx_keep_tempfile_flag.patch

diff -Nuarp nginx-0.9.5/src/http/ngx_http_core_module.c
nginx-0.9.5_new/src/http/ngx_http_core_module.c
— nginx-0.9.5/src/http/ngx_http_core_module.c 2011-01-20
07:31:24.000000000 -0800
+++ nginx-0.9.5_new/src/http/ngx_http_core_module.c 2011-03-09
07:22:45.000000000 -0800
@@ -2230,6 +2230,7 @@ ngx_http_subrequest(ngx_http_request_t *

 sr->subrequest_in_memory = (flags & NGX_HTTP_SUBREQUEST_IN_MEMORY)

!= 0;
sr->waited = (flags & NGX_HTTP_SUBREQUEST_WAITED) != 0;

sr->keep_temp_file = (flags & NGX_HTTP_REQUEST_KEEP_TEMP_FILE) !=
0;

sr->unparsed_uri = r->unparsed_uri;
sr->method_name = ngx_http_core_get_method;
diff -Nuarp nginx-0.9.5/src/http/ngx_http_request.h
nginx-0.9.5_new/src/http/ngx_http_request.h
— nginx-0.9.5/src/http/ngx_http_request.h 2011-01-20
02:37:58.000000000 -0800
+++ nginx-0.9.5_new/src/http/ngx_http_request.h 2011-03-09
07:21:25.000000000 -0800
@@ -61,6 +61,8 @@
#define NGX_HTTP_SUBREQUEST_IN_MEMORY 2
#define NGX_HTTP_SUBREQUEST_WAITED 4
#define NGX_HTTP_LOG_UNSAFE 8
+#define NGX_HTTP_REQUEST_KEEP_TEMP_FILE 16

#define NGX_HTTP_OK 200
@@ -461,6 +463,7 @@ struct ngx_http_request_s {

 unsigned                          subrequest_in_memory:1;
 unsigned                          waited:1;

unsigned keep_temp_file:1;

#if (NGX_HTTP_CACHE)
unsigned

Posted at Nginx Forum:

magz · March 9, 2011, 8:30pm

Forgot include the check to the patch

cat ngx_keep_tempfile_flag.patch
diff -Nuarp nginx-0.9.5/src/http/ngx_http_core_module.c
nginx-0.9.5_new/src/http/ngx_http_core_module.c
— nginx-0.9.5/src/http/ngx_http_core_module.c 2011-01-20
07:31:24.000000000 -0800
+++ nginx-0.9.5_new/src/http/ngx_http_core_module.c 2011-03-09
07:22:45.000000000 -0800
@@ -2230,6 +2230,7 @@ ngx_http_subrequest(ngx_http_request_t *

 sr->subrequest_in_memory = (flags & NGX_HTTP_SUBREQUEST_IN_MEMORY)

!= 0;
sr->waited = (flags & NGX_HTTP_SUBREQUEST_WAITED) != 0;

sr->keep_temp_file = (flags & NGX_HTTP_REQUEST_KEEP_TEMP_FILE) !=
0;

sr->unparsed_uri = r->unparsed_uri;
sr->method_name = ngx_http_core_get_method;
diff -Nuarp nginx-0.9.5/src/http/ngx_http_request.h
nginx-0.9.5_new/src/http/ngx_http_request.h
— nginx-0.9.5/src/http/ngx_http_request.h 2011-01-20
02:37:58.000000000 -0800
+++ nginx-0.9.5_new/src/http/ngx_http_request.h 2011-03-09
07:21:25.000000000 -0800
@@ -61,6 +61,8 @@
#define NGX_HTTP_SUBREQUEST_IN_MEMORY 2
#define NGX_HTTP_SUBREQUEST_WAITED 4
#define NGX_HTTP_LOG_UNSAFE 8
+#define NGX_HTTP_REQUEST_KEEP_TEMP_FILE 16

#define NGX_HTTP_OK 200
@@ -461,6 +463,7 @@ struct ngx_http_request_s {

 unsigned                          subrequest_in_memory:1;
 unsigned                          waited:1;

unsigned keep_temp_file:1;

#if (NGX_HTTP_CACHE)
unsigned cached:1;
— nginx-0.9.5/src/http/ngx_http_upstream.c 2010-07-28
08:56:56.000000000 -0700
+++ nginx-0.9.5_new/src/http/ngx_http_upstream.c 2011-03-09
07:30:30.000000000 -0800
@@ -2030,7 +2033,7 @@ ngx_http_upstream_send_response(ngx_http

 u->header_sent = 1;

if (r->request_body && r->request_body->temp_file) {

if (r->request_body && r->request_body->temp_file &&
!r->keep_temp_file) {
ngx_pool_run_cleanup_file(r->pool,
r->request_body->temp_file->file.fd);
r->request_body->temp_file->file.fd = NGX_INVALID_FILE;
}

Posted at Nginx Forum:

magz · March 10, 2011, 3:57am

Additional flag to ngx_http_subrequest() can’t help if you aren’t
even going to execute subrequests (as in filter finalization case)
and can’t help with ssi either, as subrequests are executed after
main request already cleared request body.

Maxim D.

Hm. I skipped this part. Sorry.

If it’s flag in struct ngx_http_request_s() then you can control inside
your module who will do the cleaning stuff, right?
So that you can set it to main request or to sub-request.

Posted at Nginx Forum:

magz · March 10, 2011, 1:29am

Hello!

On Wed, Mar 09, 2011 at 02:28:32PM -0500, magz wrote:

Forgot include the check to the patch

[…]

Please re-read what I wrote. Sometimes reading make sense, not
only writing. Thank you.

Maxim D.