cosocket timeout handling

[avi...@adallom.com]

Hi,

Currently, when using TCP cosocket's connect, receive and send methods, if a timeout error occurs, the socket is closed.

While this is a correct behavior for connect, and may be acceptable in send, this doesn't seem to me like the correct behavior for receive.

In BSD sockets, when using recv, you pass the number of bytes that you'd like to receive. The return value is the number of bytes actually received, which can either be the number bytes requested or a lower number.

In case of a lower nonzero number, this means that the receive timeout has expired. This doesn't mean that the other size has actually closed the connection. On the contrary - the other side may send more data.

If the other side closes the connection due to a shutdown/close/error, then recv should return 0.

Is there a reason for the cosocket's receive API to be different?

Thanks

Aviram

[agentzh]

Hello!

On Mon, May 13, 2013 at 7:21 AM, aviram wrote:
> Currently, when using TCP cosocket's connect, receive and send methods, if a
> timeout error occurs, the socket is closed.
> While this is a correct behavior for connect, and may be acceptable in send,
> this doesn't seem to me like the correct behavior for receive.
>

[...]

>
> Is there a reason for the cosocket's receive API to be different?
>

I agree that cosockets should not automatically close the connection
when receive timeout errors happen. And LuaSocket (which ngx_lua's
cosocket API tries to follow) does not close the connection in this
case either.

Patches welcome! :)

Best regards,
-agentzh

[avi...@adallom.com]

The following is a patch that basically adds a boolean parameter to ngx_http_lua_socket_error_retval_handler that states whether or not to close the socket. It has a few more minor changes required for the socket not to be closed on read timeout.

--- a/lua-nginx-module/src/ngx_http_lua_socket_tcp.c

+++ b/lua-nginx-module/src/ngx_http_lua_socket_tcp.c

@@ -67,6 +67,8 @@ static void ngx_http_lua_socket_resolve_handler(ngx_resolver_ctx_t *ctx);

 static int ngx_http_lua_socket_resolve_retval_handler(ngx_http_request_t *r,

     ngx_http_lua_socket_tcp_upstream_t *u, lua_State *L);

 static int ngx_http_lua_socket_error_retval_handler(ngx_http_request_t *r,

+    ngx_http_lua_socket_tcp_upstream_t *u, lua_State *L, ngx_int_t close_socket);

+static int ngx_http_lua_socket_error_retval_handler_prepare_retvals(ngx_http_request_t *r,

     ngx_http_lua_socket_tcp_upstream_t *u, lua_State *L);

 static ngx_int_t ngx_http_lua_socket_read_all(void *data, ssize_t bytes);

 static ngx_int_t ngx_http_lua_socket_read_until(void *data, ssize_t bytes);

@@ -640,7 +642,7 @@ ngx_http_lua_socket_resolve_handler(ngx_resolver_ctx_t *ctx)

                         ngx_resolver_strerror(ctx->state));

         lua_concat(L, 2);

 

-        u->prepare_retvals = ngx_http_lua_socket_error_retval_handler;

+        u->prepare_retvals = ngx_http_lua_socket_error_retval_handler_prepare_retvals;

         ngx_http_lua_socket_handle_error(r, u,

                                          NGX_HTTP_LUA_SOCKET_FT_RESOLVER);

 

@@ -806,7 +808,7 @@ ngx_http_lua_socket_resolve_retval_handler(ngx_http_request_t *r,

                    "lua tcp socket connect: %i", rc);

 

     if (rc == NGX_ERROR) {

-        return ngx_http_lua_socket_error_retval_handler(r, u, L);

+        return ngx_http_lua_socket_error_retval_handler(r, u, L, 1);

     }

 

     if (rc == NGX_BUSY) {

@@ -820,7 +822,7 @@ ngx_http_lua_socket_resolve_retval_handler(ngx_http_request_t *r,

         dd("socket errno: %d", (int) ngx_socket_errno);

         u->ft_type |= NGX_HTTP_LUA_SOCKET_FT_ERROR;

         u->socket_errno = ngx_socket_errno;

-        return ngx_http_lua_socket_error_retval_handler(r, u, L);

+        return ngx_http_lua_socket_error_retval_handler(r, u, L, 1);

     }

 

     /* rc == NGX_OK || rc == NGX_AGAIN */

@@ -878,7 +880,7 @@ ngx_http_lua_socket_resolve_retval_handler(ngx_http_request_t *r,

             ngx_http_lua_socket_handle_error(r, u,

                                              NGX_HTTP_LUA_SOCKET_FT_ERROR);

             lua_pushnil(L);

-            lua_pushliteral(L, "failed to handle write event");

+            lua_pushliteral(L, "failed to handle read event");

             return 2;

         }

 

@@ -921,7 +923,7 @@ ngx_http_lua_socket_resolve_retval_handler(ngx_http_request_t *r,

 

 static int

 ngx_http_lua_socket_error_retval_handler(ngx_http_request_t *r,

-    ngx_http_lua_socket_tcp_upstream_t *u, lua_State *L)

+    ngx_http_lua_socket_tcp_upstream_t *u, lua_State *L, ngx_int_t close_socket)

 {

     u_char           errstr[NGX_MAX_ERROR_STR];

     u_char          *p;

@@ -933,7 +935,9 @@ ngx_http_lua_socket_error_retval_handler(ngx_http_request_t *r,

         u->co_ctx->cleanup = NULL;

     }

 

-    ngx_http_lua_socket_tcp_finalize(r, u);

+    if (close_socket) {

+        ngx_http_lua_socket_tcp_finalize(r, u);

+    }

 

     if (u->ft_type & NGX_HTTP_LUA_SOCKET_FT_RESOLVER) {

         return 2;

@@ -943,6 +947,7 @@ ngx_http_lua_socket_error_retval_handler(ngx_http_request_t *r,

 

     if (u->ft_type & NGX_HTTP_LUA_SOCKET_FT_TIMEOUT) {

         lua_pushliteral(L, "timeout");

+        u->ft_type = 0;

 

     } else if (u->ft_type & NGX_HTTP_LUA_SOCKET_FT_CLOSED) {

         lua_pushliteral(L, "closed");

@@ -976,13 +981,19 @@ ngx_http_lua_socket_error_retval_handler(ngx_http_request_t *r,

     return 2;

 }

 

+static int

+ngx_http_lua_socket_error_retval_handler_prepare_retvals(ngx_http_request_t *r,

+    ngx_http_lua_socket_tcp_upstream_t *u, lua_State *L)

+{

+    return ngx_http_lua_socket_error_retval_handler(r, u, L, 1);

+}

 

 static int

 ngx_http_lua_socket_tcp_connect_retval_handler(ngx_http_request_t *r,

     ngx_http_lua_socket_tcp_upstream_t *u, lua_State *L)

 {

     if (u->ft_type) {

-        return ngx_http_lua_socket_error_retval_handler(r, u, L);

+        return ngx_http_lua_socket_error_retval_handler(r, u, L, 1);

     }

 

     lua_pushinteger(L, 1);

@@ -1384,11 +1395,11 @@ ngx_http_lua_socket_tcp_read(ngx_http_request_t *r,

 

                         if (rc == NGX_HTTP_CLIENT_CLOSED_REQUEST) {

                             ngx_http_lua_socket_handle_error(r, u,

-                                         NGX_HTTP_LUA_SOCKET_FT_CLIENTABORT);

+                                                             NGX_HTTP_LUA_SOCKET_FT_CLIENTABORT);

 

                         } else {

                             ngx_http_lua_socket_handle_error(r, u,

-                                             NGX_HTTP_LUA_SOCKET_FT_ERROR);

+                                                             NGX_HTTP_LUA_SOCKET_FT_ERROR);

                         }

 

                         return NGX_ERROR;

@@ -1398,7 +1409,7 @@ ngx_http_lua_socket_tcp_read(ngx_http_request_t *r,

 #if 1

                 if (ngx_handle_read_event(rev, 0) != NGX_OK) {

                     ngx_http_lua_socket_handle_error(r, u,

-                                     NGX_HTTP_LUA_SOCKET_FT_ERROR);

+                                                     NGX_HTTP_LUA_SOCKET_FT_ERROR);

                     return NGX_ERROR;

                 }

 #endif

@@ -1718,7 +1729,7 @@ ngx_http_lua_socket_tcp_send(lua_State *L)

     dd("socket send returned %d", (int) rc);

 

     if (rc == NGX_ERROR) {

-        return ngx_http_lua_socket_error_retval_handler(r, u, L);

+        return ngx_http_lua_socket_error_retval_handler(r, u, L, 1);

     }

 

     if (rc == NGX_OK) {

@@ -1759,7 +1770,7 @@ ngx_http_lua_socket_tcp_send_retval_handler(ngx_http_request_t *r,

                    "lua tcp socket send return value handler");

 

     if (u->ft_type) {

-        return ngx_http_lua_socket_error_retval_handler(r, u, L);

+        return ngx_http_lua_socket_error_retval_handler(r, u, L, 1);

     }

 

     lua_pushinteger(L, u->request_len);

@@ -1777,6 +1788,7 @@ ngx_http_lua_socket_tcp_receive_retval_handler(ngx_http_request_t *r,

     ngx_event_t                 *ev;

 

     ngx_http_lua_loc_conf_t             *llcf;

+    ngx_int_t                    close_socket;

 

     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,

                    "lua tcp socket receive return value handler");

@@ -1811,6 +1823,11 @@ ngx_http_lua_socket_tcp_receive_retval_handler(ngx_http_request_t *r,

 #endif

 

     if (u->ft_type) {

+        if (u->ft_type & NGX_HTTP_LUA_SOCKET_FT_TIMEOUT) {

+            close_socket = 0;

+        } else {

+            close_socket = 1;

+        }

 

         dd("u->bufs_in: %p", u->bufs_in);

 

@@ -1822,14 +1839,14 @@ ngx_http_lua_socket_tcp_receive_retval_handler(ngx_http_request_t *r,

                 return 2;

             }

 

-            (void) ngx_http_lua_socket_error_retval_handler(r, u, L);

+            (void) ngx_http_lua_socket_error_retval_handler(r, u, L, close_socket);

 

             lua_pushvalue(L, -3);

             lua_remove(L, -4);

             return 3;

         }

 

-        n = ngx_http_lua_socket_error_retval_handler(r, u, L);

+        n = ngx_http_lua_socket_error_retval_handler(r, u, L, close_socket);

         lua_pushliteral(L, "");

         return n + 1;

     }

@@ -2179,7 +2196,7 @@ ngx_http_lua_socket_handle_success(ngx_http_request_t *r,

 

 static void

 ngx_http_lua_socket_handle_error(ngx_http_request_t *r,

-    ngx_http_lua_socket_tcp_upstream_t *u, ngx_uint_t ft_type)

+                                 ngx_http_lua_socket_tcp_upstream_t *u, ngx_uint_t ft_type)

 {

     ngx_http_lua_ctx_t          *ctx;

[agentzh]

Hello!

On Thu, May 16, 2013 at 2:04 AM, aviram wrote:
> The following is a patch that basically adds a boolean parameter to
> ngx_http_lua_socket_error_retval_handler that states whether or not to close
> the socket. It has a few more minor changes required for the socket not to
> be closed on read timeout.
>

Thank you for the patch! But it seems to me that adding an extra
function argument results in code that is not really readable. How
about adding a 1-bit "fatal" field to
ngx_http_lua_socket_tcp_upstream_t?

Also, will you mind adding some test cases to the test suite to
actually exercise this change, especially for the case where
(incomplete) pending data left by a timed out receive() call in the
receive buffers?

Thanks!
-agentzh

[Yichun Zhang (agentzh)]

Hello!

On Thu, May 16, 2013 at 2:04 AM, aviram wrote:

> The following is a patch that basically adds a boolean parameter to
> ngx_http_lua_socket_error_retval_handler that states whether or not to close
> the socket. It has a few more minor changes required for the socket not to
> be closed on read timeout.
>

After fixing one bug in your patch, changing the extra function call
argument to a flag in ngx_http_lua_socket_tcp_upstream_t, and adding
quite a few test cases to the test suite, I've submitted a modified
version of the patch to ngx_lua's git master:

https://github.com/chaoslawful/lua-nginx-module/commit/2926596

You're very welcome to review and test it on your side.

Thank you for your contribution and sorry about the delay on my side!

Best regards,
-agentzh