From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 972AD42EF5C for ; Tue, 23 Jun 2020 00:23:50 +0300 (MSK) References: <20200622183433.64739-1-arkholga@tarantool.org> From: Vladislav Shpilevoy Message-ID: <8857facf-e076-c09d-bd8c-1d4187c3406d@tarantool.org> Date: Mon, 22 Jun 2020 23:23:48 +0200 MIME-Version: 1.0 In-Reply-To: <20200622183433.64739-1-arkholga@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] netbox: fix empty error message List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia , tarantool-patches@dev.tarantool.org Cc: alexander.turenko@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')