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

[Roman Khabibov <roman.habibov@tarantool.org>]

> On Jul 9, 2019, at 11:04, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
>>> The commit message should mention Roman's commit and cleanly state that
>>> (rc < 0) assumption does not work on Mac OS.
> 
> This is not done.
    lua: return getaddrinfo() errors
    
    Before this patch, branch when getaddrinfo() returns error codes
    couldn't be reached on Mac OS, because they are greater than 0 on
    Mac OS (assumption "rc < 0" in commit ea1da04d5 is incorret for
    Mac OS). Add this errors into the socket and the net.box modules.
    Now getaddrinfo() can return a pair of values (nil and error
    message) in case of error.
    
    Closes #4138

>> +	/* gh-4138: Check getaddrinfo() error. */
>> +	isnt(coio_getaddrinfo("hostname", port, NULL, &i, 1), 0, "getaddrinfo error");
> 
> The line is longer then 80 symbols
>> +	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
>> +	     "getaddrinfo error");
> 
> It would be good to have different case names.
+	/* gh-4138: Check getaddrinfo() error. */
+	isnt(coio_getaddrinfo("non_exists_hostname", port, NULL, &i, 1), 0,
+	     "getaddrinfo error");
+	isnt(strstr(diag_get()->last->errmsg, "getaddrinfo"), NULL,
+	     "getaddrinfo error message");
+

> The problem that stated in the issue is about improper error message. I don't
> understand the test in this context: it does not check whether it is so. It is
> applicable to lua tests too.
>I would add relevant test case for coio_getaddrinfo() into
>test/unit/coio.cc.
I added this for this reason.
Why? Now, error message is proper, and I check it by grep:
+-- gh-4138 Check getaddrinfo() error. Error code and error message
+-- returned by getaddrinfo() depends on system's gai_strerror()
+-- and compiler. So that there is no checking for certain error
+-- message.
+
+s, err = socket:connect('non_exists_hostname:3301')
+string.find(err, 'getaddrinfo') ~= nil
+s, err = socket:bind('non_exists_hostname:3301')
+string.find(err, 'getaddrinfo') ~= nil
+
Same about net.box test.
>> + * 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 (fist is nil, second is error
> 
> Typo: fist -> first.
> 
>> + * message).
>> + * @retval One of luaT_error() codes.
> 
> 'luaT_error() code' is the strange phrase. I guess you want to say that
> the function can raise a lua error. It is not a return value, because a
> function does not return at all in the case (it is longjmp to be
> precise).
+/**
+ * Wrap coio_getaddrinfo() and call it. Push returned values onto
+ * @a L Lua stack. In case of error raise a Lua error.
+ *
+ * @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).
+ */

>>>> +s, err = socket:connect('hostname:3301');
>>> 
>>> I would use less generic hostname like 'non_exists_hostname’.
> 
> The comment is still actual for test/unit/coio.cc.
Done.

> (I have copied parts of your patch below to comment them.)
> 
>> @@ -1347,10 +1350,10 @@ 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)
>> +    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
>>     if dns == nil or #dns == 0 then
>> -        self._errno = boxerrno.EINVAL
>> -        return nil, socket_error(self)
>> +         self._errno = boxerrno.EINVAL
> 
> Broken indent.
@@ -1347,10 +1356,10 @@ 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)
+    local dns, err = getaddrinfo(host, port, timeout, ga_opts)
     if dns == nil or #dns == 0 then
         self._errno = boxerrno.EINVAL
-        return nil, socket_error(self)
+        return nil, err
     end
     for _, remote in ipairs(dns) do
         timeout = deadline - fiber.clock()

>> @@ -1534,9 +1537,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
> 
> Why not always set errno to EINVAL and return err, nil in case on an error? It
> seems the 'if' here is redundant.
I think that if we remove this ‘if', the behaviour of these methods
(lsocket_connect(), lsocket_bind()) will be strange. In some cases error
message is present, in some cases it is not, because tcp_connect() can return
just nil without err.

> Also applicable to similar changes in other lua functions.
> 
>>     for i, remote in pairs(dns) do
>>         timeout = stop - fiber.clock()
>>         if timeout <= 0 then
>>             boxerrno(boxerrno.ETIMEDOUT)
>> -            return nil
>> +            return nil, 'timed out'
>>         end
>>         local s = socket_new(remote.family, remote.type, remote.protocol)
>>         if s then
> 
> Do you really think this change is self-explanatory and it is obvious
> why this is needed (and why this was not needed before)?
> 
> Yep, we discussed it voicely, but I'm not a last reader of your code.
-            return nil
+            -- Second arg added to do the behaviour of this
+            -- function symmetrical in case of timeout on both
+            -- Linux and Mac OS. On Mac OS error message 'timed
+            -- out' is thrown by getaddrinfo(). On Linux the
+            -- behaviour of getaddrinfo() in this case isn't same
+            -- and timeout is checked here.
+            return nil, 'timed out'