Re: [Tarantool-patches] [PATCH] netbox: fix empty error message

[Olga Arkhangelskaia <arkholga@tarantool.org>]

Hi! Thanks for the review and proposed changes!

Have squashed and updated branch.

23.06.2020 0:23, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
> See 5 comments below, a patch on top the branch and in the end of
> the email.
>
> On 22/06/2020 20:34, Olga Arkhangelskaia wrote:
>> When the connection was not established yet netbox reported empty error
>> while executing a remote request.
>> Closes #4787
>> ---
>> Branch OKriw/gh-4787-netbox-reports-empty-error
>>   
>> src/box/lua/net_box.lua                       |  6 ++--
>>   test/app/gh-4787-netbox-empty-errmsg.result   | 34 +++++++++++++++++++
>>   test/app/gh-4787-netbox-empty-errmsg.test.lua | 15 ++++++++
>>   3 files changed, 53 insertions(+), 2 deletions(-)
>>   create mode 100644 test/app/gh-4787-netbox-empty-errmsg.result
>>   create mode 100755 test/app/gh-4787-netbox-empty-errmsg.test.lua
>>
>> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
>> index 9560bfdd4..6774729b4 100644
>> --- a/src/box/lua/net_box.lua
>> +++ b/src/box/lua/net_box.lua
>> @@ -541,8 +541,10 @@ local function create_transport(host, port, user, password, callback,
>>       local function perform_async_request(buffer, skip_header, method, on_push,
>>                                            on_push_ctx, request_ctx, ...)
>>           if state ~= 'active' and state ~= 'fetch_schema' then
>> -            return nil, box.error.new({code = last_errno or E_NO_CONNECTION,
>> -                                       reason = last_error})
>> +            return nil, box.error.new({code = last_error or E_NO_CONNECTION,
>> +                                       reason = last_error or
>> +                                       string.format("connection is not eshatblished, state: %s",
>> +                                       state)})
> 1. The code block is really bad formatted. Also there are typos in the word
> 'eshatblished'.
>
>>           end
>>           -- alert worker to notify it of the queued outgoing data;
>>           -- if the buffer wasn't empty, assume the worker was already alerted
>> diff --git a/test/app/gh-4787-netbox-empty-errmsg.result b/test/app/gh-4787-netbox-empty-errmsg.result
>> new file mode 100644
>> index 000000000..9e14cfb19
>> --- /dev/null
>> +++ b/test/app/gh-4787-netbox-empty-errmsg.result
> 2. The test passes even without the fix.
>
>> @@ -0,0 +1,34 @@
>> +-- test-run result file version 2
>> +netbox = require('net.box')
>> + | ---
>> + | ...
>> +--
>> +--gh-4787:netbox reported empty error message while executing remote call
>> +--
>> +box.schema.user.grant('guest', 'execute', 'universe')
>> + | ---
>> + | ...
>> +ok, err = nil
>> + | ---
>> + | ...
>> +-- Due to race when wait_connected = false, run whole block to get an error
>> +do                                                              \
>> +   c = netbox.connect(box.cfg.listen, {wait_connected = false}) \
>> +   ok, err = pcall(c.call, c, 'any', {}, {is_async = true})     \
>> +end
>> + | ---
>> + | ...
>> +err ~= nil
>> + | ---
>> + | - true
>> + | ...
>> +err:unpack().message ~= nil
>> + | ---
>> + | - true
>> + | ...
>> +c:close()
>> + | ---
>> + | ...
>> +box.schema.user.revoke('guest', 'read,write,execute,create', 'universe')
>> + | ---
>> + | ...
>> diff --git a/test/app/gh-4787-netbox-empty-errmsg.test.lua b/test/app/gh-4787-netbox-empty-errmsg.test.lua
>> new file mode 100755
>> index 000000000..2949f16ea
>> --- /dev/null
>> +++ b/test/app/gh-4787-netbox-empty-errmsg.test.lua
>> @@ -0,0 +1,15 @@
>> +netbox = require('net.box')
>> +--
>> +--gh-4787:netbox reported empty error message while executing remote call
> 3. Please, use whitespaces after symbols like ',', ':', '--', etc. Also
> end sentences using the dot, and keep the comments in 66 line width.
>
>> +--
>> +box.schema.user.grant('guest', 'execute', 'universe')
>> +ok, err = nil
>> +-- Due to race when wait_connected = false, run whole block to get an error
>> +do                                                              \
>> +   c = netbox.connect(box.cfg.listen, {wait_connected = false}) \
> 4. Indentation step in Lua code is 4 spaces, not 3.
>
>> +   ok, err = pcall(c.call, c, 'any', {}, {is_async = true})     \
> 5. You are calling a not existing function. So the error message can actually
> contain something about this type of error instead of the not established
> connection. I know I used the test in the issue, but it is not perfect.
>
>> +end
>> +err ~= nil
>> +err:unpack().message ~= nil
>> +c:close()
>> +box.schema.user.revoke('guest', 'read,write,execute,create', 'universe')
> Consider the diff below, which I also pushed on top of the branch.
>
> ====================
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 6774729b4..70bba0d6f 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -541,10 +541,11 @@ local function create_transport(host, port, user, password, callback,
>       local function perform_async_request(buffer, skip_header, method, on_push,
>                                            on_push_ctx, request_ctx, ...)
>           if state ~= 'active' and state ~= 'fetch_schema' then
> -            return nil, box.error.new({code = last_error or E_NO_CONNECTION,
> -                                       reason = last_error or
> -                                       string.format("connection is not eshatblished, state: %s",
> -                                       state)})
> +            local code = last_errno or E_NO_CONNECTION
> +            local msg = last_error or
> +                string.format('Connection is not established, state is "%s"',
> +                              state)
> +            return nil, box.error.new({code = code, reason = msg})
>           end
>           -- alert worker to notify it of the queued outgoing data;
>           -- if the buffer wasn't empty, assume the worker was already alerted
> diff --git a/test/app/gh-4787-netbox-empty-errmsg.result b/test/app/gh-4787-netbox-empty-errmsg.result
> index 72d2d08e0..d30337a05 100644
> --- a/test/app/gh-4787-netbox-empty-errmsg.result
> +++ b/test/app/gh-4787-netbox-empty-errmsg.result
> @@ -2,32 +2,60 @@
>   netbox = require('net.box')
>    | ---
>    | ...
> +fiber = require('fiber')
> + | ---
> + | ...
>   --
> ---gh-4787:netbox reported empty error message while executing remote call
> +-- gh-4787: netbox reported empty error message while executing
> +-- remote call.
>   --
> -box.schema.user.grant('guest', 'execute', 'universe')
> +box.schema.user.create('test', { password = 'test' })
>    | ---
>    | ...
> -ok, err = nil
> +box.schema.user.grant('test', 'super')
>    | ---
>    | ...
> -do                                                              \
> -   c = netbox.connect(box.cfg.listen, {wait_connected = false}) \
> -   ok, err = pcall(c.call, c, 'any', {}, {is_async = true})     \
> +function echo(...) return ... end
> + | ---
> + | ...
> +
> +-- Check that a request in 'auth' state returns a correct error.
> +function req_during_auth()                                                      \
> +    local c = netbox.connect(box.cfg.listen, {                                  \
> +        user = 'test', password = 'test', wait_connected = false                \
> +    })                                                                          \
> +    while c.state ~= 'auth' do fiber.yield() end                                \
> +    local ok, err = pcall(c.call, c, 'echo', {}, {is_async = true})             \
> +    c:close()                                                                   \
> +    return ok, err                                                              \
>   end
>    | ---
>    | ...
> -err ~= nil
> +
> +req_during_auth()
> + | ---
> + | - false
> + | - Connection is not established, state is "auth"
> + | ...
> +
> +-- Check the same for 'initial' state.
> +ok, err = nil
> + | ---
> + | ...
> +do                                                                              \
> +    c = netbox.connect(box.cfg.listen, {wait_connected = false})                \
> +    ok, err = pcall(c.call, c, 'echo', {}, {is_async = true})                   \
> +end
>    | ---
> - | - true
>    | ...
> -err:unpack().message ~= nil
> +ok, err
>    | ---
> - | - true
> + | - false
> + | - Connection is not established, state is "initial"
>    | ...
>   c:close()
>    | ---
>    | ...
> -box.schema.user.revoke('guest', 'read,write,execute,create', 'universe')
> +box.schema.user.drop('test')
>    | ---
>    | ...
> diff --git a/test/app/gh-4787-netbox-empty-errmsg.test.lua b/test/app/gh-4787-netbox-empty-errmsg.test.lua
> index 41ab3fec0..0eecaa1bf 100755
> --- a/test/app/gh-4787-netbox-empty-errmsg.test.lua
> +++ b/test/app/gh-4787-netbox-empty-errmsg.test.lua
> @@ -1,14 +1,32 @@
>   netbox = require('net.box')
> +fiber = require('fiber')
>   --
> ---gh-4787:netbox reported empty error message while executing remote call
> +-- gh-4787: netbox reported empty error message while executing
> +-- remote call.
>   --
> -box.schema.user.grant('guest', 'execute', 'universe')
> +box.schema.user.create('test', { password = 'test' })
> +box.schema.user.grant('test', 'super')
> +function echo(...) return ... end
> +
> +-- Check that a request in 'auth' state returns a correct error.
> +function req_during_auth()                                                      \
> +    local c = netbox.connect(box.cfg.listen, {                                  \
> +        user = 'test', password = 'test', wait_connected = false                \
> +    })                                                                          \
> +    while c.state ~= 'auth' do fiber.yield() end                                \
> +    local ok, err = pcall(c.call, c, 'echo', {}, {is_async = true})             \
> +    c:close()                                                                   \
> +    return ok, err                                                              \
> +end
> +
> +req_during_auth()
> +
> +-- Check the same for 'initial' state.
>   ok, err = nil
> -do                                                              \
> -   c = netbox.connect(box.cfg.listen, {wait_connected = false}) \
> -   ok, err = pcall(c.call, c, 'any', {}, {is_async = true})     \
> +do                                                                              \
> +    c = netbox.connect(box.cfg.listen, {wait_connected = false})                \
> +    ok, err = pcall(c.call, c, 'echo', {}, {is_async = true})                   \
>   end
> -err ~= nil
> -err:unpack().message ~= nil
> +ok, err
>   c:close()
> -box.schema.user.revoke('guest', 'read,write,execute,create', 'universe')
> +box.schema.user.drop('test')