[Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area
Nikita Pettik
korablev at tarantool.org
Wed Apr 1 19:26:18 MSK 2020
On 31 Mar 01:24, Vladislav Shpilevoy wrote:
> Thanks for the patchset!
>
> On future - it would be good to have diffs and answers on
> my comments inlined. Otherwise I need to jump between the
> new email, the old one, and the commit's diff to check what
> and how was fixed. And I likely forget something.
It's quite complicated to extract certain diff while working
on whole patch-set and changes are big enough. Will try next time.
(All non-optional requested fixes are done; I answered explicitly
on skipped comments).
> See 10 comments below.
>
> On 25/03/2020 02:43, Nikita Pettik wrote:
> > This patch introduces support of stacked errors in IProto protocol and
> > in net.box module.
> >
> > @TarantoolBot document
> > Title: Stacked error diagnostic area
> >
> > Starting from now errors can be organized into lists. To achieve this
> > Lua table representing error object is extended with .prev field and
> > e:set_prev(err) method. .prev field returns previous error if any exist.
> > e:set_prev(err) method expects err to be error object or nil and sets
> > err as previous error of e. For instance:
> >
> > e1 = box.error.new({code = 111, reason = "cause"})
> > e2 = box.error.new({code = 111, reason = "cause of cause"})
>
> 1. I would wrap these code samples into ```. Otherwise you may
> screw the whole ticket formatting accidentally. You can check how
> will it look by clicking 'New issue', pasting the request, and
> clicking 'Preview'.
Fixed.
> >
> > Closes #1148
>
> 2. Propose to move this to before the docbot request. So as not
> to include it into the doc ticket.
Fixed.
> > diff --git a/src/box/xrow.c b/src/box/xrow.c
> > index 256dd4d91..ceb38b040 100644
> > --- a/src/box/xrow.c
> > +++ b/src/box/xrow.c
> > @@ -1070,19 +1083,66 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len,
> > return 0;
> > }
> >
> > +static int
> > +iproto_decode_error_stack(const char **pos, const char *end)
> > +{
> > + const char *reason = NULL;
> > + static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\
> > + "expected to be larger than error message max length");
> > + /*
> > + * Erase previously set diag errors. It is required since
> > + * box_error_add() does not replace previous errors.
> > + */
> > + box_error_clear();
> > + if (mp_check_array(*pos, end) != 0)
>
> 3. mp_check() functions return > 0 on error.
>
> When you call mp_check_array() without checking if it is
> MP_ARRAY, it will crash. Also it will crash if *pos == end
> in the checks below. But it does not matter anymore.
>
> I noticed that we don't need MessagePack validness checking
> here. I see now that mp_check() is called by some upper function
> always. You need only check that MP_ types are correct. That it
> is MP_ARRAY, keys are MP_UINT and so on.
Fixed:
diff --git a/src/box/xrow.c b/src/box/xrow.c
index e84c6f758..596c765b7 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1084,7 +1084,7 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len,
}
static int
-iproto_decode_error_stack(const char **pos, const char *end)
+iproto_decode_error_stack(const char **pos)
{
const char *reason = NULL;
static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\
@@ -1099,22 +1099,19 @@ iproto_decode_error_stack(const char **pos, const char *end)
uint32_t stack_sz = mp_decode_array(pos);
for (uint32_t i = 0; i < stack_sz; i++) {
uint32_t code = 0;
- if (mp_typeof(**pos) != MP_MAP || mp_check_map(*pos, end) != 0)
+ if (mp_typeof(**pos) != MP_MAP)
return -1;
uint32_t map_sz = mp_decode_map(pos);
for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
- if (mp_typeof(**pos) != MP_UINT ||
- mp_check_uint(*pos, end) != 0)
+ if (mp_typeof(**pos) != MP_UINT)
return -1;
uint8_t key = mp_decode_uint(pos);
if (key == IPROTO_ERROR_CODE) {
- if (mp_typeof(**pos) != MP_UINT ||
- mp_check_uint(*pos, end) != 0)
+ if (mp_typeof(**pos) != MP_UINT)
return -1;
code = mp_decode_uint(pos);
} else if (key == IPROTO_ERROR_MESSAGE) {
- if (mp_typeof(**pos) != MP_STR ||
- mp_check_strl(*pos, end) != 0)
+ if (mp_typeof(**pos) != MP_STR)
return -1;
uint32_t len;
const char *str = mp_decode_str(pos, &len);
@@ -1169,7 +1166,7 @@ xrow_decode_error(struct xrow_header *row)
} else if (key == IPROTO_ERROR_STACK ) {
if (mp_check_array(pos, end) != 0)
goto error;
- if (iproto_decode_error_stack(&pos, end) != 0)
+ if (iproto_decode_error_stack(&pos) != 0)
continue;
} else {
mp_next(&pos);
> I don't see any tests on that. Are there some? Maybe at least
> unit tests, in xrow.cc? Although a proper test would try to
> send an error stack through applier. Because xrow_decode_error()
> is used only there.
I'm a bit new to replication tests. I've been working on it.
Could you please review the rest of changes so far?
> > diff --git a/test/box/iproto.result b/test/box/iproto.result
> > new file mode 100644
> > index 000000000..bb369be5a
> > --- /dev/null
> > +++ b/test/box/iproto.result
> > @@ -0,0 +1,141 @@
> > +test_run = require('test_run').new()
>
> 4. test_run objects seems to be not necessary here.
Reworked whole iproto.test.lua according to your suggestion
to use separate function setting error stack. See diff below.
> > +...
> > +IPROTO_ERROR_MESSAGE = 0x02
> > +---
> > +...
> > +IPROTO_OK = 0x00
> > +---
> > +...
> > +IPROTO_SCHEMA_VERSION = 0x05
> > +---
> > +...
> > +IPROTO_STATUS_KEY = 0x00
>
> 5. The last 3 keys are unused.
For sure. Removed.
> > +...
> > +assert(response.body[IPROTO_ERROR] ~= nil)
> > +---
> > +- true
> > +...
> > +err = response.body[IPROTO_ERROR_STACK][1]
> > +---
> > +...
> > +assert(err[IPROTO_ERROR_MESSAGE] == body[IPROTO_ERROR])
> > +---
> > +- error: assertion failed!
>
> 6.Shouldn't it be true? You meant response.body, not just body?
Yep, fixed. Thx.
> > +---
> > +- true
> > +...
> > +assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string')
>
> 7. This test covers the previous assertion. type(nil)
> is nil, so it won't be equal to 'string'.
Didn't get it. It was not nil, but string containing message.
Nevermind, test is reworked.
> > diff --git a/test/box/net.box.result b/test/box/net.box.result
> > index e3dabf7d9..eeefc0e3f 100644
> > --- a/test/box/net.box.result
> > +++ b/test/box/net.box.result
> > @@ -3902,6 +3902,66 @@ sock:close()
> > ---
> > - true
> > ...
> > +-- gh-1148: test stacked diagnostics.
> > +--
> > +test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
>
> 8. I removed the filter and nothing changed. Why do you
> need it?
This test has been reworked as well.
> 9. Functional index is an overkill for such a simple test. You
> may prefer creating a stored function, which creates a stack of 2
> errors, and calls box.error(). That would be simpler IMO, and you can
> test more than 2 errors too. The same for the iproto.test.lua. To get
> that error you can use IPROTO_CALL. No need to create spaces, indexes,
> bother with some 'determenistic sandboxed' shit. No even need to call
> box.schema.func.create assuming you have 'universe execute' access.
diff --git a/test/box/iproto.result b/test/box/iproto.result
index d6411d71a..b6dc7ed4c 100644
--- a/test/box/iproto.result
+++ b/test/box/iproto.result
@@ -7,16 +7,19 @@ urilib = require('uri')
msgpack = require('msgpack')
---
...
-IPROTO_REQUEST_TYPE = 0x00
+test_run = require('test_run').new()
---
...
-IPROTO_INSERT = 0x02
+IPROTO_REQUEST_TYPE = 0x00
---
...
IPROTO_SYNC = 0x01
---
...
-IPROTO_SPACE_ID = 0x10
+IPROTO_CALL = 0x0A
+---
+...
+IPROTO_FUNCTION_NAME = 0x22
---
...
IPROTO_TUPLE = 0x21
@@ -36,20 +39,25 @@ IPROTO_ERROR_MESSAGE = 0x02
...
-- gh-1148: test capabilities of stacked diagnostics bypassing net.box.
--
-lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
----
-...
-box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true})
----
-...
-s = box.schema.space.create('s')
+test_run:cmd("setopt delimiter ';'")
---
+- true
...
-pk = s:create_index('pk')
+stack_err = function()
+ local e1 = box.error.new({code = 111, reason = "e1"})
+ local e2 = box.error.new({code = 111, reason = "e2"})
+ local e3 = box.error.new({code = 111, reason = "e3"})
+ assert(e1 ~= nil)
+ e2:set_prev(e1)
+ assert(e2.prev == e1)
+ e3:set_prev(e2)
+ box.error(e3)
+end;
---
...
-idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
+test_run:cmd("setopt delimiter ''");
---
+- true
...
box.schema.user.grant('guest', 'read, write, execute', 'universe')
---
@@ -58,14 +66,14 @@ next_request_id = 16
---
...
header = { \
- [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \
+ [IPROTO_REQUEST_TYPE] = IPROTO_CALL, \
[IPROTO_SYNC] = next_request_id, \
}
---
...
body = { \
- [IPROTO_SPACE_ID] = s.id, \
- [IPROTO_TUPLE] = box.tuple.new({1}) \
+ [IPROTO_FUNCTION_NAME] = 'stack_err', \
+ [IPROTO_TUPLE] = box.tuple.new({nil}) \
}
---
...
@@ -99,31 +107,36 @@ assert(err[IPROTO_ERROR_MESSAGE] == response.body[IPROTO_ERROR])
---
- true
...
-err = response.body[IPROTO_ERROR_STACK][2]
+assert(err[IPROTO_ERROR_MESSAGE] == 'e3')
---
+- true
...
-assert(err[IPROTO_ERROR_CODE] ~= nil)
+assert(err[IPROTO_ERROR_CODE] == 111)
---
- true
...
-assert(type(err[IPROTO_ERROR_CODE]) == 'number')
+err = response.body[IPROTO_ERROR_STACK][2]
---
-- true
...
-assert(err[IPROTO_ERROR_MESSAGE] ~= nil)
+assert(err[IPROTO_ERROR_MESSAGE] == 'e2')
---
- true
...
-assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string')
+assert(err[IPROTO_ERROR_CODE] == 111)
---
- true
...
-box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+err = response.body[IPROTO_ERROR_STACK][3]
---
...
-s:drop()
+assert(err[IPROTO_ERROR_MESSAGE] == 'e1')
---
+- true
...
-box.func.f1:drop()
+assert(err[IPROTO_ERROR_CODE] == 111)
+---
+- true
+...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
diff --git a/test/box/iproto.test.lua b/test/box/iproto.test.lua
index d0b55467b..6402a22ba 100644
--- a/test/box/iproto.test.lua
+++ b/test/box/iproto.test.lua
@@ -1,11 +1,13 @@
net_box = require('net.box')
urilib = require('uri')
msgpack = require('msgpack')
+test_run = require('test_run').new()
IPROTO_REQUEST_TYPE = 0x00
-IPROTO_INSERT = 0x02
+
IPROTO_SYNC = 0x01
-IPROTO_SPACE_ID = 0x10
+IPROTO_CALL = 0x0A
+IPROTO_FUNCTION_NAME = 0x22
IPROTO_TUPLE = 0x21
IPROTO_ERROR = 0x31
IPROTO_ERROR_STACK = 0x52
@@ -14,24 +16,29 @@ IPROTO_ERROR_MESSAGE = 0x02
-- gh-1148: test capabilities of stacked diagnostics bypassing net.box.
--
-lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
-
-box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true})
-s = box.schema.space.create('s')
-pk = s:create_index('pk')
-idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
-
+test_run:cmd("setopt delimiter ';'")
+stack_err = function()
+ local e1 = box.error.new({code = 111, reason = "e1"})
+ local e2 = box.error.new({code = 111, reason = "e2"})
+ local e3 = box.error.new({code = 111, reason = "e3"})
+ assert(e1 ~= nil)
+ e2:set_prev(e1)
+ assert(e2.prev == e1)
+ e3:set_prev(e2)
+ box.error(e3)
+end;
+test_run:cmd("setopt delimiter ''");
box.schema.user.grant('guest', 'read, write, execute', 'universe')
next_request_id = 16
header = { \
- [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \
+ [IPROTO_REQUEST_TYPE] = IPROTO_CALL, \
[IPROTO_SYNC] = next_request_id, \
}
body = { \
- [IPROTO_SPACE_ID] = s.id, \
- [IPROTO_TUPLE] = box.tuple.new({1}) \
+ [IPROTO_FUNCTION_NAME] = 'stack_err', \
+ [IPROTO_TUPLE] = box.tuple.new({nil}) \
}
uri = urilib.parse(box.cfg.listen)
@@ -47,12 +54,13 @@ assert(response.body[IPROTO_ERROR] ~= nil)
err = response.body[IPROTO_ERROR_STACK][1]
assert(err[IPROTO_ERROR_MESSAGE] == response.body[IPROTO_ERROR])
+assert(err[IPROTO_ERROR_MESSAGE] == 'e3')
+assert(err[IPROTO_ERROR_CODE] == 111)
err = response.body[IPROTO_ERROR_STACK][2]
-assert(err[IPROTO_ERROR_CODE] ~= nil)
-assert(type(err[IPROTO_ERROR_CODE]) == 'number')
-assert(err[IPROTO_ERROR_MESSAGE] ~= nil)
-assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string')
+assert(err[IPROTO_ERROR_MESSAGE] == 'e2')
+assert(err[IPROTO_ERROR_CODE] == 111)
+err = response.body[IPROTO_ERROR_STACK][3]
+assert(err[IPROTO_ERROR_MESSAGE] == 'e1')
+assert(err[IPROTO_ERROR_CODE] == 111)
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
-s:drop()
-box.func.f1:drop()
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 764eef2c4..6475707ef 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3904,63 +3904,74 @@ sock:close()
...
-- gh-1148: test stacked diagnostics.
--
-test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+test_run:cmd("setopt delimiter ';'")
---
- true
...
-lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
+stack_err = function()
+ local e1 = box.error.new({code = 111, reason = "e1"})
+ local e2 = box.error.new({code = 111, reason = "e2"})
+ local e3 = box.error.new({code = 111, reason = "e3"})
+ assert(e1 ~= nil)
+ e2:set_prev(e1)
+ assert(e2.prev == e1)
+ e3:set_prev(e2)
+ box.error(e3)
+end;
---
...
-box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+test_run:cmd("setopt delimiter ''");
---
+- true
...
-s = box.schema.space.create('s')
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
---
...
-pk = s:create_index('pk')
+c = net.connect(box.cfg.listen)
---
...
-idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
+f = function(...) return c:call(...) end
---
...
-box.schema.user.grant('guest', 'read,write,execute', 'universe')
+r, e3 = pcall(f, 'stack_err')
---
...
-c = net.connect(box.cfg.listen)
+assert(r == false)
---
+- true
...
-f = function(...) return c.space.s:insert(...) end
+e3
---
+- e3
...
-_, e = pcall(f, {1})
+e2 = e3.prev
---
...
-assert(e ~= nil)
+assert(e2 ~= nil)
---
- true
...
-e:unpack().message
+e2
---
-- 'Failed to build a key for functional index ''idx'' of space ''s'': can''t evaluate
- function'
+- e2
...
-e.prev.message
+e1 = e2.prev
---
-- '[string "return function(tuple) local json = require(''..."]:1: attempt to call
- global ''require'' (a nil value)'
...
-box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+assert(e1 ~= nil)
---
+- true
...
-s:drop()
+e1
---
+- e1
...
-box.func.f1:drop()
+assert(e1.prev == nil)
---
+- true
...
-test_run:cmd("clear filter")
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
-- true
...
test_run:wait_log('default', 'Got a corrupted row.*', nil, 10)
---
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 7e3571d43..72a4207ee 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1576,26 +1576,34 @@ sock:close()
-- gh-1148: test stacked diagnostics.
--
-test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
-lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
-box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
-s = box.schema.space.create('s')
-pk = s:create_index('pk')
-idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
+test_run:cmd("setopt delimiter ';'")
+stack_err = function()
+ local e1 = box.error.new({code = 111, reason = "e1"})
+ local e2 = box.error.new({code = 111, reason = "e2"})
+ local e3 = box.error.new({code = 111, reason = "e3"})
+ assert(e1 ~= nil)
+ e2:set_prev(e1)
+ assert(e2.prev == e1)
+ e3:set_prev(e2)
+ box.error(e3)
+end;
+test_run:cmd("setopt delimiter ''");
box.schema.user.grant('guest', 'read,write,execute', 'universe')
c = net.connect(box.cfg.listen)
-f = function(...) return c.space.s:insert(...) end
-_, e = pcall(f, {1})
-assert(e ~= nil)
-
-e:unpack().message
-e.prev.message
+f = function(...) return c:call(...) end
+r, e3 = pcall(f, 'stack_err')
+assert(r == false)
+e3
+e2 = e3.prev
+assert(e2 ~= nil)
+e2
+e1 = e2.prev
+assert(e1 ~= nil)
+e1
+assert(e1.prev == nil)
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
-s:drop()
-box.func.f1:drop()
-test_run:cmd("clear filter")
test_run:wait_log('default', 'Got a corrupted row.*', nil, 10)
test_run:wait_log('default', '00000000:.*', nil, 10)
> > +---
> > +...
> > +box.schema.user.grant('guest', 'read,write,execute', 'universe')
> > +---
> > +...
> > +c = net.connect(box.cfg.listen)
> > +---
> > +...
> > +f = function(...) return c.space.s:insert(...) end
> > +---
> > +...
> > +_, e = pcall(f, {1})
> > +---
> > +...
> > +assert(e ~= nil)
> > +---
> > +- true
> > +...
> > +e:unpack().message
> > +---
> > +- 'Failed to build a key for functional index ''idx'' of space ''s'': can''t evaluate
> > + function'
> > +...
> > +e.prev.message
> > +---
> > +- error: '[string "return e.prev.message "]:1: attempt to index field ''prev'' (a
> > + nil value)'
>
> 10. Something is wrong here, no?
Yes. After rebase on master branch, new IPROTO_ERROR_STACK code
has changed, but I forgot to updated net.box sources. Fixed.
More information about the Tarantool-patches
mailing list