Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch, please see below.

Sergos

On 12 мар 13:24, Roman Khabibov wrote:
> Add getaddrinfo() errors into the several fuctions of socket. Now
> getaddrinfo() can return a pair of values (nil and error message)
> in case of error.
> 
> Closes #4138
> 
> @TarantoolBot document
> Title: socket API changes
> 
> * socket.getaddrinfo()
> 
> Can return error message as second return value in case of
> internal getaddrinfo() error.
I really don't like the case 'can return'. Can we work this out to
always return a pair of values?

> 
> Example:
> tarantool> socket.getaddrinfo('non_exists_hostname', 3301)
> ---
> - null
> - 'getaddrinfo: nodename nor servname provided, or not known'
> ...
> 
> * socket.tcp_connect()
> 
> Can return socket.getaddrinfo() error message as second return
> value.
> 
> Example:
> tarantool> socket.tcp_connect('non_exists_hostname', 3301)
> ---
> - null
> - 'getaddrinfo: nodename nor servname provided, or not known'
> ...
> ---
>  src/box/lua/net_box.lua   |  7 +++--
>  src/lua/socket.c          | 16 +++++++++-
>  src/lua/socket.lua        | 22 ++++++++++----
>  test/app/socket.result    | 63 ++++++++++++++++++++++++++++++++++++---
>  test/app/socket.test.lua  | 35 ++++++++++++++++++++--
>  test/box/net.box.result   | 31 +++++++++++++++++++
>  test/box/net.box.test.lua | 20 +++++++++++++
>  7 files changed, 179 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 3f611c027..8068a8637 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -154,9 +154,12 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
>  local function establish_connection(host, port, timeout)
>      local timeout = timeout or DEFAULT_CONNECT_TIMEOUT
>      local begin = fiber.clock()
> -    local s = socket.tcp_connect(host, port, timeout)
> +    local s, err = socket.tcp_connect(host, port, timeout)
>      if not s then
> -        return nil, errno.strerror(errno())
> +        if not err then
> +            return nil, errno.strerror(errno())
> +        end
> +        return nil, err
>      end
>      local msg = s:read({chunk = IPROTO_GREETING_SIZE},
>                          timeout - (fiber.clock() - begin))
> diff --git a/src/lua/socket.c b/src/lua/socket.c
> index e75a8802e..ba683ad3a 100644
> --- a/src/lua/socket.c
> +++ b/src/lua/socket.c
> @@ -775,6 +775,18 @@ lbox_getaddrinfo_result_wrapper(struct lua_State *L)
>  	return 1;
>  }
>  
> +/**
> + * Wrap coio_getaddrinfo() and call it. Push returned values onto
> + * @a L Lua stack.
> + *
> + * @param L Lua stack.
> + *
> + * @retval 1 Number of returned values by Lua function if
> + * coio_getaddrinfo() success.
> + * @retval 2 Number of returned values by Lua function if
> + * coio_getaddrinfo() is failed (first is nil, second is error
> + * message).
> + */
>  static int
>  lbox_socket_getaddrinfo(struct lua_State *L)
>  {
> @@ -817,7 +829,9 @@ lbox_socket_getaddrinfo(struct lua_State *L)
>  
>  	if (dns_res != 0) {
>  		lua_pushnil(L);
> -		return 1;
> +		struct error *err = diag_get()->last;
> +		lua_pushstring(L, err->errmsg);
> +		return 2;
>  	}
>  
>  	/* no results */
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index a334ad45b..ce6dab301 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -1041,9 +1041,12 @@ local function tcp_connect(host, port, timeout)
>      end
>      local timeout = timeout or TIMEOUT_INFINITY
>      local stop = fiber.clock() + timeout
> -    local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
> +    local dns, err = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM',
>          protocol = 'tcp' })
> -    if dns == nil or #dns == 0 then
> +    if dns == nil then
> +        return nil, err
You explicitly put error here...

> +    end
> +    if #dns == 0 then
>          boxerrno(boxerrno.EINVAL)
>          return nil
... and not here. Why do you keep using two versions of error passing,
not one? I would agree you need boxerrno for backward compatibility, but
to start moving towards {res, err} you have to introduce it everywhere. 

>      end
> @@ -1360,8 +1363,12 @@ local function lsocket_tcp_connect(self, host, port)
>      -- This function is broken by design
>      local ga_opts = { family = 'AF_INET', type = 'SOCK_STREAM' }
>      local timeout = deadline - fiber.clock()
> -    local dns = getaddrinfo(host, port, timeout, ga_opts)
> -    if dns == nil or #dns == 0 then
> +    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
> +    if dns == nil then
> +        self._errno = boxerrno.EINVAL
> +        return nil, err
> +    end
> +    if #dns == 0 then
>          self._errno = boxerrno.EINVAL
>          return nil, socket_error(self)
>      end
> @@ -1547,9 +1554,12 @@ local function lsocket_connect(host, port)
>      if host == nil or port == nil then
>          error("Usage: luasocket.connect(host, port)")
>      end
> -    local s = tcp_connect(host, port)
> +    local s, err = tcp_connect(host, port)
>      if not s then
> -        return nil, boxerrno.strerror()
> +        if not err then
> +            return nil, boxerrno.strerror()
> +        end
> +        return nil, err
>      end
>      setmetatable(s, lsocket_tcp_client_mt)
>      return s
> diff --git a/test/app/socket.result b/test/app/socket.result
> index 9f0ea0a5d..3f26bf2b0 100644
> --- a/test/app/socket.result
> +++ b/test/app/socket.result
> @@ -941,10 +941,20 @@ sc:close()
>  - true
>  ...
>  -- tcp_connect
> --- test timeout
> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
> +-- Test timeout. In this test, tcp_connect can return the second
> +-- output value from internal.getaddrinfo (usually on Mac OS, but
> +-- theoretically it can happen on Linux too). Sometimes
> +-- getaddrinfo() is timed out, sometimes connect. On Linux however
> +-- getaddrinfo is fast enough to never give timeout error in
> +-- the case. So, there are two sources of timeout errors that are
> +-- reported differently. This difference has appeared after
> +-- gh-4138 patch.
> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
>  ---
> -- null
> +...
> +s == nil
> +---
> +- true
>  ...
>  -- AF_INET
>  s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
> @@ -1595,7 +1605,7 @@ fio.stat(path) == nil
>  { socket.tcp_connect('abrakadabra#123') == nil, errno.strerror() }
>  ---
>  - - true
> -  - Invalid argument
> +  - Input/output error
>  ...
>  -- wrong options for getaddrinfo
>  socket.getaddrinfo('host', 'port', { type = 'WRONG' }) == nil and errno() == errno.EINVAL
> @@ -2914,3 +2924,48 @@ srv:close()
>  ---
>  - true
>  ...
> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> +-- Error code and error message returned by getaddrinfo() depends
> +-- on system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +        return true
> +    end
> +    return false
> +end;
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +s, err = socket.getaddrinfo('non_exists_hostname', 3301)
> +---
> +...
> +check_err(err)
> +---
> +- true
> +...
> +s, err = socket.connect('non_exists_hostname', 3301)
> +---
> +...
> +check_err(err)
> +---
> +- true
> +...
> +s, err = socket.tcp_connect('non_exists_hostname', 3301)
> +---
> +...
> +check_err(err)
> +---
> +- true
> +...
> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
> index d1fe7ada6..c455d3ddb 100644
> --- a/test/app/socket.test.lua
> +++ b/test/app/socket.test.lua
> @@ -300,8 +300,16 @@ sc:close()
>  
>  -- tcp_connect
>  
> --- test timeout
> -socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
> +-- Test timeout. In this test, tcp_connect can return the second
> +-- output value from internal.getaddrinfo (usually on Mac OS, but
> +-- theoretically it can happen on Linux too). Sometimes
> +-- getaddrinfo() is timed out, sometimes connect. On Linux however
> +-- getaddrinfo is fast enough to never give timeout error in
> +-- the case. So, there are two sources of timeout errors that are
> +-- reported differently. This difference has appeared after
> +-- gh-4138 patch.
> +s, err = socket.tcp_connect('127.0.0.1', 80, 0.00000000001)
> +s == nil
>  
>  -- AF_INET
>  s = socket('AF_INET', 'SOCK_STREAM', 'tcp')
> @@ -988,3 +996,26 @@ s:receive('*a')
>  s:close()
>  srv:close()
>  
> +-- gh-4138 Check getaddrinfo() error from socket:connect() only.
> +-- Error code and error message returned by getaddrinfo() depends
> +-- on system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +        return true
> +    end
> +    return false
> +end;
I really doubt that different error messages from the set above will appear 
for the same error on different platforms. Could you please check for particular 
output for each case you trigger below?

> +test_run:cmd("setopt delimiter ''");
> +
> +s, err = socket.getaddrinfo('non_exists_hostname', 3301)
> +check_err(err)
> +s, err = socket.connect('non_exists_hostname', 3301)
> +check_err(err)
> +s, err = socket.tcp_connect('non_exists_hostname', 3301)
> +check_err(err)
> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index e3dabf7d9..3e1ce6b11 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -3927,6 +3927,37 @@ test_run:grep_log('default', '00000040:.*')
>  ---
>  - null
>  ...
> +-- gh-4138 Check getaddrinfo() error from connect() only. Error
> +-- code and error message returned by getaddrinfo() depends on
> +-- system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +            return true
> +    end
> +    return false
> +end;
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +s = remote.connect('non_exists_hostname:3301')
> +---
> +...
> +check_err(s['error'])
> +---
> +- true
> +...
>  box.cfg{log_level=log_level}
>  ---
>  ...
> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
> index 8e65ff470..cad8754c3 100644
> --- a/test/box/net.box.test.lua
> +++ b/test/box/net.box.test.lua
> @@ -1582,4 +1582,24 @@ test_run:wait_log('default', '00000030:.*', nil, 10)
>  -- we expect nothing below, so don't wait
>  test_run:grep_log('default', '00000040:.*')
>  
> +-- gh-4138 Check getaddrinfo() error from connect() only. Error
> +-- code and error message returned by getaddrinfo() depends on
> +-- system's gai_strerror().
> +test_run:cmd("setopt delimiter ';'")
> +function check_err(err)
> +    if err == 'getaddrinfo: nodename nor servname provided, or not known' or
> +       err == 'getaddrinfo: Servname not supported for ai_socktype' or
> +       err == 'getaddrinfo: Name or service not known' or
> +       err == 'getaddrinfo: hostname nor servname provided, or not known' or
> +       err == 'getaddrinfo: Temporary failure in name resolution' or
> +       err == 'getaddrinfo: Name could not be resolved at this time' then
> +            return true
> +    end
> +    return false
> +end;
> +test_run:cmd("setopt delimiter ''");
> +
> +s = remote.connect('non_exists_hostname:3301')
> +check_err(s['error'])
> +
>  box.cfg{log_level=log_level}
> -- 
> 2.21.0 (Apple Git-122)
>