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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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')