Forum: NGINX use a return code for ngx_http_close_request()

2974d09ac2541e892966b762aad84943?d=identicon&s=25 crespin (Guest)
on 2014-06-27 15:26
(Received via mailing list)
Hello,

here is another path still on ngx_http_request.c.
In function ngx_http_close_request(), the second parameter is an error
code.

This error code is used in ngx_http_free_request() to set the HTTP
status
code if it's not present or if no bytes are already sent.

Use NGX_OK instead of zero seems - for me - valid.

When ngx_http_close_request() is called after an error, I guess it's
must be
NGX_HTTP_INTERNAL_SERVER_ERROR.

Perhaps, it's better to do two patch one for zero to NGX_OK and another
for
NGX_HTTP_INTERNAL_SERVER_ERROR.

Thanks for your comments,

Best regards,

yves

Subject: [PATCH] use an error code for ngx_http_close_request()

---
 1.0/source/nginx-1.7.2/src/http/ngx_http_request.c |   38
++++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/1.0/source/nginx-1.7.2/src/http/ngx_http_request.c
b/1.0/source/nginx-1.7.2/src/http/ngx_http_request.c
index 8ddfc60..1979229 100644
--- a/1.0/source/nginx-1.7.2/src/http/ngx_http_request.c
+++ b/1.0/source/nginx-1.7.2/src/http/ngx_http_request.c
@@ -2429,7 +2429,7 @@ ngx_http_finalize_request(ngx_http_request_t *r,
ngx_int_t rc)
     }

     if (c->read->eof) {
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_OK);
         return;
     }

@@ -2493,7 +2493,7 @@ ngx_http_terminate_handler(ngx_http_request_t *r)

     r->count = 1;

-    ngx_http_close_request(r, 0);
+    ngx_http_close_request(r, NGX_OK);
 }


@@ -2504,7 +2504,7 @@ ngx_http_finalize_connection(ngx_http_request_t
*r)

 #if (NGX_HTTP_SPDY)
     if (r->spdy_stream) {
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_OK);
         return;
     }
 #endif
@@ -2523,7 +2523,7 @@ ngx_http_finalize_connection(ngx_http_request_t
*r)
             }
         }

-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_OK);
         return;
     }

@@ -2546,7 +2546,7 @@ ngx_http_finalize_connection(ngx_http_request_t
*r)
         return;
     }

-    ngx_http_close_request(r, 0);
+    ngx_http_close_request(r, NGX_OK);
 }


@@ -2581,7 +2581,7 @@ ngx_http_set_write_handler(ngx_http_request_t *r)
     }

     if (ngx_handle_write_event(wev, clcf->send_lowat) != NGX_OK) {
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
         return NGX_ERROR;
     }

@@ -2622,7 +2622,7 @@ ngx_http_writer(ngx_http_request_t *r)
             ngx_add_timer(wev, clcf->send_timeout);

             if (ngx_handle_write_event(wev, clcf->send_lowat) !=
NGX_OK) {
-                ngx_http_close_request(r, 0);
+                ngx_http_close_request(r,
NGX_HTTP_INTERNAL_SERVER_ERROR);
             }

             return;
@@ -2635,7 +2635,7 @@ ngx_http_writer(ngx_http_request_t *r)
                        "http writer delayed");

         if (ngx_handle_write_event(wev, clcf->send_lowat) != NGX_OK) {
-            ngx_http_close_request(r, 0);
+            ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
         }

         return;
@@ -2659,7 +2659,7 @@ ngx_http_writer(ngx_http_request_t *r)
         }

         if (ngx_handle_write_event(wev, clcf->send_lowat) != NGX_OK) {
-            ngx_http_close_request(r, 0);
+            ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
         }

         return;
@@ -2696,7 +2696,7 @@ ngx_http_block_reading(ngx_http_request_t *r)
         && r->connection->read->active)
     {
         if (ngx_del_event(r->connection->read, NGX_READ_EVENT, 0) !=
NGX_OK) {
-            ngx_http_close_request(r, 0);
+            ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
         }
     }
 }
@@ -2798,7 +2798,7 @@ ngx_http_test_reading(ngx_http_request_t *r)
     if ((ngx_event_flags & NGX_USE_LEVEL_EVENT) && rev->active) {

         if (ngx_del_event(rev, NGX_READ_EVENT, 0) != NGX_OK) {
-            ngx_http_close_request(r, 0);
+            ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
         }
     }

@@ -2869,7 +2869,7 @@ ngx_http_set_keepalive(ngx_http_request_t *r)
                   cscf->large_client_header_buffers.num *
sizeof(ngx_buf_t
*));

                 if (hc->free == NULL) {
-                    ngx_http_close_request(r, 0);
+                    ngx_http_close_request(r,
NGX_HTTP_INTERNAL_SERVER_ERROR);
                     return;
                 }
             }
@@ -3198,7 +3198,7 @@ ngx_http_set_lingering_close(ngx_http_request_t
*r)
     ngx_add_timer(rev, clcf->lingering_timeout);

     if (ngx_handle_read_event(rev, 0) != NGX_OK) {
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r,
NGX_HTTP_INTERNAL_SERVER_ERRORNGX_ERROR);
         return;
     }

@@ -3207,7 +3207,7 @@ ngx_http_set_lingering_close(ngx_http_request_t
*r)

     if (wev->active && (ngx_event_flags & NGX_USE_LEVEL_EVENT)) {
         if (ngx_del_event(wev, NGX_WRITE_EVENT, 0) != NGX_OK) {
-            ngx_http_close_request(r, 0);
+            ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
             return;
         }
     }
@@ -3215,7 +3215,7 @@ ngx_http_set_lingering_close(ngx_http_request_t
*r)
     if (ngx_shutdown_socket(c->fd, NGX_WRITE_SHUTDOWN) == -1) {
         ngx_connection_error(c, ngx_socket_errno,
                              ngx_shutdown_socket_n " failed");
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
         return;
     }

@@ -3242,13 +3242,13 @@ ngx_http_lingering_close_handler(ngx_event_t
*rev)
                    "http lingering close handler");

     if (rev->timedout) {
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_OK);
         return;
     }

     timer = (ngx_msec_t) r->lingering_time - (ngx_msec_t) ngx_time();
     if ((ngx_msec_int_t) timer <= 0) {
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_OK);
         return;
     }

@@ -3258,14 +3258,14 @@ ngx_http_lingering_close_handler(ngx_event_t
*rev)
         ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0, "lingering read:
%d",
n);

         if (n == NGX_ERROR || n == 0) {
-            ngx_http_close_request(r, 0);
+            ngx_http_close_request(r, (n == 0) ? NGX_OK : NGX_ERROR);
             return;
         }

     } while (rev->ready);

     if (ngx_handle_read_event(rev, 0) != NGX_OK) {
-        ngx_http_close_request(r, 0);
+        ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
         return;
     }

--
1.7.10.4

Posted at Nginx Forum:
http://forum.nginx.org/read.php?2,251228,251228#msg-251228
A8108a0961c6087c43cda32c8616dcba?d=identicon&s=25 Maxim Dounin (Guest)
on 2014-06-27 21:42
(Received via mailing list)
Hello!

On Fri, Jun 27, 2014 at 09:26:23AM -0400, crespin wrote:

> Hello,
>
> here is another path still on ngx_http_request.c.
> In function ngx_http_close_request(), the second parameter is an error
> code.
>
> This error code is used in ngx_http_free_request() to set the HTTP status
> code if it's not present or if no bytes are already sent.
>
> Use NGX_OK instead of zero seems - for me - valid.

The same logic applies as in the previous answer about
ngx_http_terminate_request().

> When ngx_http_close_request() is called after an error, I guess it's must be
> NGX_HTTP_INTERNAL_SERVER_ERROR.
>
> Perhaps, it's better to do two patch one for zero to NGX_OK and another for
> NGX_HTTP_INTERNAL_SERVER_ERROR.

Use of NGX_HTTP_INTERNAL_SERVER_ERROR is wrong, as there are no
chances that this response code will be ever actually sent.
Response headers are either already sent, or won't be sent at all.

--
Maxim Dounin
http://nginx.org/
2974d09ac2541e892966b762aad84943?d=identicon&s=25 crespin (Guest)
on 2014-06-28 12:54
(Received via mailing list)
Maxim Dounin Wrote:
-------------------------------------------------------
> >
> must be
> --
> Maxim Dounin
> http://nginx.org/
>
> _______________________________________________
> nginx mailing list
> nginx@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx

Hello Maxim,

Thanks for your response.

ngx_hhtp_close_request() call ngx_http_free_request()
Reading the code with your comments, rc is only set when a timeout is
raised.
Is it right ?
So the parameter can be a flag ngx_flag_t is_timeout ?

yves

Posted at Nginx Forum:
http://forum.nginx.org/read.php?2,251228,251281#msg-251281
A8108a0961c6087c43cda32c8616dcba?d=identicon&s=25 Maxim Dounin (Guest)
on 2014-06-30 21:00
(Received via mailing list)
Hello!

On Sat, Jun 28, 2014 at 06:54:14AM -0400, crespin wrote:

> ngx_hhtp_close_request() call ngx_http_free_request()
> Reading the code with your comments, rc is only set when a timeout is
> raised.
> Is it right ?
> So the parameter can be a flag ngx_flag_t is_timeout ?

I don't understand your question, sorry.

--
Maxim Dounin
http://nginx.org/
Please log in before posting. Registration is free and takes only a minute.
Existing account

NEW: Do you have a Google/GoogleMail, Yahoo or Facebook account? No registration required!
Log in with Google account | Log in with Yahoo account | Log in with Facebook account
No account? Register here.