Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area

[Nikita Pettik <korablev@tarantool.org>]

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.