From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 EE28742EF5C for ; Tue, 23 Jun 2020 14:24:25 +0300 (MSK) References: <20200622183433.64739-1-arkholga@tarantool.org> <8857facf-e076-c09d-bd8c-1d4187c3406d@tarantool.org> From: Olga Arkhangelskaia Message-ID: <60aed029-3e92-70a4-8f4e-dd7aa0f3ae26@tarantool.org> Date: Tue, 23 Jun 2020 14:24:12 +0300 MIME-Version: 1.0 In-Reply-To: <8857facf-e076-c09d-bd8c-1d4187c3406d@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB 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: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Cc: alexander.turenko@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')