[Tarantool-patches] [PATCH] netbox: fix empty error message
Olga Arkhangelskaia
arkholga at tarantool.org
Tue Jun 23 14:24:12 MSK 2020
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')
More information about the Tarantool-patches
mailing list