Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Ostanevich <sergos@tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors
Date: Sun, 29 Mar 2020 12:07:18 +0300	[thread overview]
Message-ID: <20200329090718.GD328@tarantool.org> (raw)
In-Reply-To: <20200312102434.97300-3-roman.habibov@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)
>

  reply	other threads:[~2020-03-29  9:07 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-12 10:24 [Tarantool-patches] [PATCH v2 0/2] Return " Roman Khabibov
2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 1/2] coio/say: fix getaddrinfo error handling on macOS Roman Khabibov
2020-03-29  8:51   ` Sergey Ostanevich
2020-03-29  9:12     ` Alexander Turenko
2020-04-06  2:07     ` Roman Khabibov
2020-04-17  9:37       ` Sergey Ostanevich
2020-04-24 11:32         ` Roman Khabibov
2020-06-23 13:27         ` Roman Khabibov
2020-07-23 14:12           ` Sergey Ostanevich
2020-07-28  9:26             ` Roman Khabibov
2020-04-26 17:20   ` Vladislav Shpilevoy
2020-03-12 10:24 ` [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors Roman Khabibov
2020-03-29  9:07   ` Sergey Ostanevich [this message]
2020-03-29  9:25     ` Alexander Turenko
2020-04-06  2:08     ` Roman Khabibov
2020-04-16 10:27       ` Sergey Ostanevich
2020-04-24 11:42         ` Roman Khabibov
2020-04-24 17:18           ` Sergey Ostanevich
2020-04-26  3:16             ` Roman Khabibov
2020-04-26 15:46               ` Alexander Turenko
2020-04-26 16:26                 ` Roman Khabibov
2020-07-13  9:51                   ` sergos
2020-07-28 14:08 ` [Tarantool-patches] [PATCH v2 0/2] Return " Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200329090718.GD328@tarantool.org \
    --to=sergos@tarantool.org \
    --cc=roman.habibov@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 2/2] lua: return getaddrinfo() errors' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox