* [Tarantool-patches] [PATCH V4 1/6] error: add custom error type
2020-04-16 17:38 [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Leonid Vasiliev
@ 2020-04-16 17:38 ` Leonid Vasiliev
2020-04-17 0:49 ` Vladislav Shpilevoy
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 2/6] error: send custom type in IProto Leonid Vasiliev
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Leonid Vasiliev @ 2020-04-16 17:38 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
Part of #4398
@TarantoolBot document
Title: Custom error types for Lua errors
Errors can be created in 2 ways: `box.error.new()` and `box.error()`.
Both used to take either `code, reason, <reason string args>` or
`{code = code, reason = reason, ...}` arguments.
Now in the first option instead of code a user can specify a
string as its own error type. In the second option a user can
specify both code and type. For example:
```Lua
box.error('MyErrorType', 'Message')
box.error({type = 'MyErrorType', code = 1024, reason = 'Message'})
```
Or no-throw version:
```Lua
box.error.new('MyErrorType', 'Message')
box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
```
When a custom type is specified, it is shown in `err.type`
attribute. When it is not specified, `err.type` shows one of
built-in errors such as 'ClientError', 'OurOfMemory', etc.
Name length limit on the custom type is 63 bytes. All is longer
is truncated.
Original error type can be checked using `err.base_type` member,
although normally it should not be used. For user-defined types
base type is 'CustomError'.
For example:
```
tarantool> e = box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
---
...
tarantool> e:unpack()
---
- code: 1024
trace:
- file: '[string "e = box.error.new({type = ''MyErrorType'', code..."]'
line: 1
type: MyErrorType
custom_type: MyErrorType
message: Message
base_type: CustomError
...
```
---
| 1 +
src/box/errcode.h | 1 +
src/box/error.cc | 92 +++++++++++++++++++++++++++++++++----------
src/box/error.h | 36 +++++++++++++++--
src/box/lua/error.cc | 42 ++++++++++++++------
src/box/xrow.c | 2 +-
src/lua/error.lua | 14 ++++++-
test/app/fiber.result | 5 ++-
test/box/error.result | 66 +++++++++++++++++++++++++++++--
test/box/error.test.lua | 15 +++++++
test/engine/func_index.result | 14 ++++---
11 files changed, 240 insertions(+), 48 deletions(-)
--git a/extra/exports b/extra/exports
index a9add2c..9467398 100644
--- a/extra/exports
+++ b/extra/exports
@@ -235,6 +235,7 @@ box_index_min
box_index_max
box_index_count
box_error_type
+box_error_custom_type
box_error_code
box_error_message
box_error_last
diff --git a/src/box/errcode.h b/src/box/errcode.h
index d1e4d02..e37b7bd 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -266,6 +266,7 @@ struct errcode_record {
/*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \
/*212 */_(ER_SEQUENCE_NOT_STARTED, "Sequence '%s' is not started") \
/*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \
+ /*214 */_(ER_CUSTOM_ERROR, "%s") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/error.cc b/src/box/error.cc
index 233b312..c8fadfb 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -86,35 +86,50 @@ box_error_set(const char *file, unsigned line, uint32_t code,
return -1;
}
+static struct error *
+box_error_new_va(const char *file, unsigned line, uint32_t code,
+ const char *custom_type, const char *fmt, va_list ap)
+{
+ if (custom_type == NULL) {
+ struct error *e = BuildClientError(file, line, ER_UNKNOWN);
+ ClientError *client_error = type_cast(ClientError, e);
+ if (client_error != NULL) {
+ client_error->m_errcode = code;
+ error_vformat_msg(e, fmt, ap);
+ }
+ return e;
+ } else {
+ struct error *e = BuildCustomError(file, line, custom_type);
+ CustomError *custom_error = type_cast(CustomError, e);
+ if (custom_error != NULL)
+ error_vformat_msg(e, fmt, ap);
+
+ return e;
+ }
+}
+
struct error *
box_error_new(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...)
+ const char *custom_type, const char *fmt, ...)
{
- struct error *e = BuildClientError(file, line, ER_UNKNOWN);
- ClientError *client_error = type_cast(ClientError, e);
- if (client_error != NULL) {
- client_error->m_errcode = code;
- va_list ap;
- va_start(ap, fmt);
- error_vformat_msg(e, fmt, ap);
- va_end(ap);
- }
+ va_list ap;
+ va_start(ap, fmt);
+ struct error *e = box_error_new_va(file, line, code, custom_type,
+ fmt, ap);
+ va_end(ap);
return e;
}
int
box_error_add(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...)
+ const char *custom_type, const char *fmt, ...)
{
- struct error *e = BuildClientError(file, line, ER_UNKNOWN);
- ClientError *client_error = type_cast(ClientError, e);
- if (client_error) {
- client_error->m_errcode = code;
- va_list ap;
- va_start(ap, fmt);
- error_vformat_msg(e, fmt, ap);
- va_end(ap);
- }
+ va_list ap;
+ va_start(ap, fmt);
+ struct error *e = box_error_new_va(file, line, code, custom_type,
+ fmt, ap);
+ va_end(ap);
+
struct diag *d = &fiber()->diag;
if (diag_is_empty(d))
diag_set_error(d, e);
@@ -125,6 +140,16 @@ box_error_add(const char *file, unsigned line, uint32_t code,
/* }}} */
+const char *
+box_error_custom_type(const struct error *e)
+{
+ CustomError *custom_error = type_cast(CustomError, e);
+ if (custom_error)
+ return custom_error->custom_type();
+
+ return NULL;
+}
+
struct rmean *rmean_error = NULL;
const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
@@ -290,3 +315,30 @@ BuildAccessDeniedError(const char *file, unsigned int line,
return e;
}
}
+
+static struct method_info customerror_methods[] = {
+ make_method(&type_CustomError, "custom_type", &CustomError::custom_type),
+ METHODS_SENTINEL
+};
+
+const struct type_info type_CustomError =
+ make_type("CustomError", &type_ClientError, customerror_methods);
+
+CustomError::CustomError(const char *file, unsigned int line,
+ const char *custom_type)
+ :ClientError(&type_CustomError, file, line, ER_CUSTOM_ERROR)
+{
+ error_format_msg(this, tnt_errcode_desc(m_errcode), custom_type);
+ strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
+ m_custom_type[sizeof(m_custom_type) - 1] = '\0';
+}
+
+struct error *
+BuildCustomError(const char *file, unsigned int line, const char *custom_type)
+{
+ try {
+ return new CustomError(file, line, custom_type);
+ } catch (OutOfMemory *e) {
+ return e;
+ }
+}
diff --git a/src/box/error.h b/src/box/error.h
index ca5d5b2..311d543 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -53,6 +53,9 @@ struct error *
BuildXlogGapError(const char *file, unsigned line,
const struct vclock *from, const struct vclock *to);
+struct error *
+BuildCustomError(const char *file, unsigned int line, const char *custom_type);
+
/** \cond public */
struct error;
@@ -138,11 +141,21 @@ box_error_set(const char *file, unsigned line, uint32_t code,
/** \endcond public */
/**
+ * Return the error custom type,
+ * \param e Error object.
+ * \return Pointer to custom error type. On error return NULL.
+ */
+const char *
+box_error_custom_type(const struct error *e);
+
+/**
* Add error to the diagnostic area. In contrast to box_error_set()
* it does not replace previous error being set, but rather link
* them into list.
*
* \param code IPROTO error code (enum \link box_error_code \endlink)
+ * \param custom_type User-defined error type which will be
+ * displayed instead of ClientError.
* \param format (const char * ) - printf()-like format string
* \param ... - format arguments
* \returns -1 for convention use
@@ -151,19 +164,20 @@ box_error_set(const char *file, unsigned line, uint32_t code,
*/
int
box_error_add(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...);
+ const char *custom_type, const char *fmt, ...);
/**
* Construct error object without setting it in the diagnostics
- * area. On the memory allocation fail returns NULL.
+ * area. On the memory allocation fail returns OutOfMemory error.
*/
struct error *
box_error_new(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...);
+ const char *custom_type, const char *fmt, ...);
extern const struct type_info type_ClientError;
extern const struct type_info type_XlogError;
extern const struct type_info type_AccessDeniedError;
+extern const struct type_info type_CustomError;
#if defined(__cplusplus)
} /* extern "C" */
@@ -290,6 +304,22 @@ struct XlogGapError: public XlogError
virtual void raise() { throw this; }
};
+class CustomError: public ClientError
+{
+public:
+ CustomError(const char *file, unsigned int line,
+ const char *custom_type);
+
+ const char*
+ custom_type()
+ {
+ return m_custom_type;
+ }
+private:
+ /** Custom type name */
+ char m_custom_type[64];
+};
+
#endif /* defined(__cplusplus) */
#endif /* TARANTOOL_BOX_ERROR_H_INCLUDED */
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index b2625bf..68703d8 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -46,6 +46,13 @@ extern "C" {
* Parse Lua arguments (they can come as single table
* f({code : number, reason : string}) or as separate members
* f(code, reason)) and construct struct error with given values.
+ *
+ * Instead of 'code' it is possible to specify a string name of
+ * the error object's type:
+ *
+ * box.error(type, reason, ...)
+ * box.error({type = string, reason = string, ...})
+ *
* In case one of arguments is missing its corresponding field
* in struct error is filled with default value.
*/
@@ -53,13 +60,21 @@ static struct error *
luaT_error_create(lua_State *L, int top_base)
{
uint32_t code = 0;
+ const char *custom_type = NULL;
const char *reason = NULL;
const char *file = "";
unsigned line = 0;
lua_Debug info;
int top = lua_gettop(L);
- if (top >= top_base && lua_type(L, top_base) == LUA_TNUMBER) {
- code = lua_tonumber(L, top_base);
+ int top_type = lua_type(L, top_base);
+ if (top >= top_base && (top_type == LUA_TNUMBER ||
+ top_type == LUA_TSTRING)) {
+ if (top_type == LUA_TNUMBER) {
+ code = lua_tonumber(L, top_base);
+ } else {
+ code = ER_CUSTOM_ERROR;
+ custom_type = lua_tostring(L, top_base);
+ }
reason = tnt_errcode_desc(code);
if (top > top_base) {
/* Call string.format(reason, ...) to format message */
@@ -78,14 +93,19 @@ luaT_error_create(lua_State *L, int top_base)
/* Missing arguments to format string */
return NULL;
}
- } else if (top == top_base && lua_istable(L, top_base)) {
+ } else if (top == top_base && top_type == LUA_TTABLE) {
lua_getfield(L, top_base, "code");
- code = lua_tonumber(L, -1);
- lua_pop(L, 1);
+ if (!lua_isnil(L, -1))
+ code = lua_tonumber(L, -1);
+ else
+ code = ER_CUSTOM_ERROR;
lua_getfield(L, top_base, "reason");
reason = lua_tostring(L, -1);
if (reason == NULL)
reason = "";
+ lua_getfield(L, top_base, "type");
+ if (!lua_isnil(L, -1))
+ custom_type = lua_tostring(L, -1);
lua_pop(L, 1);
} else {
return NULL;
@@ -102,7 +122,7 @@ raise:
}
line = info.currentline;
}
- return box_error_new(file, line, code, "%s", reason);
+ return box_error_new(file, line, code, custom_type, "%s", reason);
}
static int
@@ -149,11 +169,11 @@ luaT_error_last(lua_State *L)
static int
luaT_error_new(lua_State *L)
{
- if (lua_gettop(L) == 0)
- return luaL_error(L, "Usage: box.error.new(code, args)");
- struct error *e = luaT_error_create(L, 1);
- if (e == NULL)
- return luaL_error(L, "Usage: box.error.new(code, args)");
+ struct error *e;
+ if (lua_gettop(L) == 0 || (e = luaT_error_create(L, 1)) == NULL) {
+ return luaL_error(L, "Usage: box.error.new(code, args) or "\
+ "box.error.new(type, args)");
+ }
lua_settop(L, 0);
luaT_pusherror(L, e);
return 1;
diff --git a/src/box/xrow.c b/src/box/xrow.c
index a494d1f..5494b41 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1123,7 +1123,7 @@ iproto_decode_error_stack(const char **pos)
continue;
}
}
- box_error_add(__FILE__, __LINE__, code, reason);
+ box_error_add(__FILE__, __LINE__, code, NULL, reason);
}
return 0;
}
diff --git a/src/lua/error.lua b/src/lua/error.lua
index bdc9c71..a5f7435 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -35,6 +35,9 @@ exception_get_int(struct error *e, const struct method_info *method);
int
error_set_prev(struct error *e, struct error *prev);
+
+const char *
+box_error_custom_type(const struct error *e);
]]
local REFLECTION_CACHE = {}
@@ -75,10 +78,18 @@ local function reflection_get(err, method)
end
end
-local function error_type(err)
+local function error_base_type(err)
return ffi.string(err._type.name)
end
+local function error_type(err)
+ local res = ffi.C.box_error_custom_type(err)
+ if res ~= nil then
+ return ffi.string(res)
+ end
+ return error_base_type(err)
+end
+
local function error_message(err)
return ffi.string(err._errmsg)
end
@@ -131,6 +142,7 @@ local error_fields = {
["trace"] = error_trace;
["errno"] = error_errno;
["prev"] = error_prev;
+ ["base_type"] = error_base_type
}
local function error_unpack(err)
diff --git a/test/app/fiber.result b/test/app/fiber.result
index debfc67..9b82790 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1038,8 +1038,9 @@ st;
...
e:unpack();
---
-- type: ClientError
- code: 1
+- code: 1
+ base_type: ClientError
+ type: ClientError
message: Illegal parameters, oh my
trace:
- file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
diff --git a/test/box/error.result b/test/box/error.result
index df6d0eb..82db453 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -34,8 +34,9 @@ e
| ...
e:unpack()
| ---
- | - type: ClientError
- | code: 1
+ | - code: 1
+ | base_type: ClientError
+ | type: ClientError
| message: Illegal parameters, bla bla
| trace:
| - file: '[C]'
@@ -105,7 +106,7 @@ e
| ...
box.error.new()
| ---
- | - error: 'Usage: box.error.new(code, args)'
+ | - error: 'Usage: box.error.new(code, args) or box.error.new(type, args)'
| ...
--
@@ -431,6 +432,7 @@ t;
| 211: box.error.WRONG_QUERY_ID
| 212: box.error.SEQUENCE_NOT_STARTED
| 213: box.error.NO_SUCH_SESSION_SETTING
+ | 214: box.error.CUSTOM_ERROR
| ...
test_run:cmd("setopt delimiter ''");
@@ -489,7 +491,7 @@ assert(box.error.last() == nil)
--
box.error.new(err)
| ---
- | - error: 'Usage: box.error.new(code, args)'
+ | - error: 'Usage: box.error.new(code, args) or box.error.new(type, args)'
| ...
-- box.error() is supposed to re-throw last diagnostic error.
@@ -831,3 +833,59 @@ assert(box.error.last() == e1)
| ---
| - true
| ...
+--
+-- gh-4398: custom error type.
+--
+-- Try no code.
+e = box.error.new({type = 'TestType', reason = 'Test reason'})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 214
+ | base_type: CustomError
+ | type: TestType
+ | custom_type: TestType
+ | message: Test reason
+ | trace:
+ | - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
+ | line: 1
+ | ...
+-- Try code not the same as used by default.
+e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 214
+ | base_type: CustomError
+ | type: TestType
+ | custom_type: TestType
+ | message: Test reason
+ | trace:
+ | - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
+ | line: 1
+ | ...
+-- Try to omit message.
+e = box.error.new({type = 'TestType'})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 214
+ | base_type: CustomError
+ | type: TestType
+ | custom_type: TestType
+ | message:
+ | trace:
+ | - file: '[string "e = box.error.new({type = ''TestType''}) "]'
+ | line: 1
+ | ...
+-- Try too long type name.
+e = box.error.new({type = string.rep('a', 128)})
+ | ---
+ | ...
+#e.type
+ | ---
+ | - 63
+ | ...
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index 41baed5..38f7406 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -229,3 +229,18 @@ box.error({code = 111, reason = "err"})
box.error.last()
box.error(e1)
assert(box.error.last() == e1)
+--
+-- gh-4398: custom error type.
+--
+-- Try no code.
+e = box.error.new({type = 'TestType', reason = 'Test reason'})
+e:unpack()
+-- Try code not the same as used by default.
+e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
+e:unpack()
+-- Try to omit message.
+e = box.error.new({type = 'TestType'})
+e:unpack()
+-- Try too long type name.
+e = box.error.new({type = string.rep('a', 128)})
+#e.type
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index a827c92..e70e591 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -277,21 +277,23 @@ e = box.error.last()
e:unpack()
| ---
| - code: 198
- | trace:
- | - file: <filename>
- | line: <line>
+ | base_type: ClientError
| type: ClientError
- | message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
- | can''t evaluate function'
| prev: '[string "return function(tuple) local ..."]:1: attempt to
| call global ''require'' (a nil value)'
+ | message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ | can''t evaluate function'
+ | trace:
+ | - file: <filename>
+ | line: <line>
| ...
e = e.prev
| ---
| ...
e:unpack()
| ---
- | - type: LuajitError
+ | - base_type: LuajitError
+ | type: LuajitError
| message: '[string "return function(tuple) local ..."]:1: attempt
| to call global ''require'' (a nil value)'
| trace:
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Tarantool-patches] [PATCH V4 1/6] error: add custom error type
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 1/6] error: add custom error type Leonid Vasiliev
@ 2020-04-17 0:49 ` Vladislav Shpilevoy
2020-04-18 17:14 ` lvasiliev
0 siblings, 1 reply; 16+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-17 0:49 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches
Thanks for the patch!
See 2 comments below.
On 16/04/2020 19:38, Leonid Vasiliev wrote:
> Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
>
> Part of #4398
>
> @TarantoolBot document
> Title: Custom error types for Lua errors
>
> Errors can be created in 2 ways: `box.error.new()` and `box.error()`.
>
> Both used to take either `code, reason, <reason string args>` or
> `{code = code, reason = reason, ...}` arguments.
>
> Now in the first option instead of code a user can specify a
> string as its own error type. In the second option a user can
> specify both code and type. For example:
>
> ```Lua
> box.error('MyErrorType', 'Message')
> box.error({type = 'MyErrorType', code = 1024, reason = 'Message'})
> ```
> Or no-throw version:
> ```Lua
> box.error.new('MyErrorType', 'Message')
> box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
> ```
> When a custom type is specified, it is shown in `err.type`
> attribute. When it is not specified, `err.type` shows one of
> built-in errors such as 'ClientError', 'OurOfMemory', etc.
>
> Name length limit on the custom type is 63 bytes. All is longer
> is truncated.
1. I changed 'All is longer' -> 'All what is longer'. Force
pushed.
> diff --git a/extra/exports b/extra/exports
> index a9add2c..9467398 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -235,6 +235,7 @@ box_index_min
> box_index_max
> box_index_count
> box_error_type
> +box_error_custom_type
> box_error_code
> box_error_message
> box_error_last
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index d1e4d02..e37b7bd 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -266,6 +266,7 @@ struct errcode_record {
> /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \
> /*212 */_(ER_SEQUENCE_NOT_STARTED, "Sequence '%s' is not started") \
> /*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \
> + /*214 */_(ER_CUSTOM_ERROR, "%s") \
2. We discussed that with Alexander - what to do with error codes.
Because there are 3 options:
- CustomErrors don't have a code. Only type name. Code will be
added later as a part of payload. In that case the syntax:
box.error.new({code = 123, type = 'MyType'})
becomes illegal. 'error.code' returns nil in Lua for CustomError
objects. And CustomError should not be a descendant of ClientError
in our code.
- CustomError has a code, it is always 214. Syntax with custom error
code is banned too. But 'error.code' is 214 always. Which is weird.
214 is a very strange number, and it creates an unnecessary link
with box.error.* codes, which are valid only for ClientErrors.
- CustomError has a code and a user can choose it. By default it is 0.
Now the patch is neither of this. You allow to specify code in
box.error.new() and box.error(), but it is silently ignored and
replaced with 214.
From what I understand, Alexander wants one of the versions above.
Certainly not the one where we ignore code argument in box.error.new()
and box.error() functions.
I am for the last option. So when you write
box.error.new({code = 123, type = 'MyType'})
You can take 'error.code' and it is 123.
That is the simplest solution in terms of implementation, and the
simplest in terms of the API. Because you have single format for all
errors:
{type = ..., code = ..., reason = ...}
Where type and code are optional. If we go for any other option,
we will need to split it in 2 versions: either
{type = ..., reason = ...}
or
{code = ..., reason = ...}
One can argue, that error codes can intersect, but the thing
is that ClientError and CustomError are different types for a user.
box.error.* codes are valid only and only for ClientError objects.
Moreover, even now I can specify not existing codes when create a
ClientError object. So intersection argument does not work - codes
can intersect even now. And it seems to be not a problem.
Just consider this example:
tarantool> args = {code = 214, reason = 'Message'}
---
...
tarantool> err = box.error.new(args)
---
...
tarantool> err:unpack()
---
- code: 214
base_type: ClientError
type: ClientError
message: Message
trace:
- file: '[string "err = box.error.new(args)"]'
line: 1
...
I created a ClientError with code 214. Not CustomError. ClientError.
It means, that code is never going to be enough to detect error type.
You will never be able to compare code with 214, and if they are
equal, say that it is CustomError. The example above proves it is not
true. You always need to look at error type first. Therefore it makes
no sense to ban codes for CustomErrors. It won't protect from anything.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Tarantool-patches] [PATCH V4 1/6] error: add custom error type
2020-04-17 0:49 ` Vladislav Shpilevoy
@ 2020-04-18 17:14 ` lvasiliev
0 siblings, 0 replies; 16+ messages in thread
From: lvasiliev @ 2020-04-18 17:14 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
On 17.04.2020 3:49, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
> On 16/04/2020 19:38, Leonid Vasiliev wrote:
>> Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
>>
>> Part of #4398
>>
>> @TarantoolBot document
>> Title: Custom error types for Lua errors
>>
>> Errors can be created in 2 ways: `box.error.new()` and `box.error()`.
>>
>> Both used to take either `code, reason, <reason string args>` or
>> `{code = code, reason = reason, ...}` arguments.
>>
>> Now in the first option instead of code a user can specify a
>> string as its own error type. In the second option a user can
>> specify both code and type. For example:
>>
>> ```Lua
>> box.error('MyErrorType', 'Message')
>> box.error({type = 'MyErrorType', code = 1024, reason = 'Message'})
>> ```
>> Or no-throw version:
>> ```Lua
>> box.error.new('MyErrorType', 'Message')
>> box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
>> ```
>> When a custom type is specified, it is shown in `err.type`
>> attribute. When it is not specified, `err.type` shows one of
>> built-in errors such as 'ClientError', 'OurOfMemory', etc.
>>
>> Name length limit on the custom type is 63 bytes. All is longer
>> is truncated.
>
> 1. I changed 'All is longer' -> 'All what is longer'. Force
> pushed.
>
Ok.
>> diff --git a/extra/exports b/extra/exports
>> index a9add2c..9467398 100644
>> --- a/extra/exports
>> +++ b/extra/exports
>> @@ -235,6 +235,7 @@ box_index_min
>> box_index_max
>> box_index_count
>> box_error_type
>> +box_error_custom_type
>> box_error_code
>> box_error_message
>> box_error_last
>> diff --git a/src/box/errcode.h b/src/box/errcode.h
>> index d1e4d02..e37b7bd 100644
>> --- a/src/box/errcode.h
>> +++ b/src/box/errcode.h
>> @@ -266,6 +266,7 @@ struct errcode_record {
>> /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \
>> /*212 */_(ER_SEQUENCE_NOT_STARTED, "Sequence '%s' is not started") \
>> /*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \
>> + /*214 */_(ER_CUSTOM_ERROR, "%s") \
>
> 2. We discussed that with Alexander - what to do with error codes.
> Because there are 3 options:
>
> - CustomErrors don't have a code. Only type name. Code will be
> added later as a part of payload. In that case the syntax:
>
> box.error.new({code = 123, type = 'MyType'})
>
> becomes illegal. 'error.code' returns nil in Lua for CustomError
> objects. And CustomError should not be a descendant of ClientError
> in our code.
>
>
> - CustomError has a code, it is always 214. Syntax with custom error
> code is banned too. But 'error.code' is 214 always. Which is weird.
> 214 is a very strange number, and it creates an unnecessary link
> with box.error.* codes, which are valid only for ClientErrors.
>
>
> - CustomError has a code and a user can choose it. By default it is 0.
>
>
> Now the patch is neither of this. You allow to specify code in
> box.error.new() and box.error(), but it is silently ignored and
> replaced with 214.
>
> From what I understand, Alexander wants one of the versions above.
> Certainly not the one where we ignore code argument in box.error.new()
> and box.error() functions.
>
> I am for the last option. So when you write
>
> box.error.new({code = 123, type = 'MyType'})
>
> You can take 'error.code' and it is 123.
>
> That is the simplest solution in terms of implementation, and the
> simplest in terms of the API. Because you have single format for all
> errors:
>
> {type = ..., code = ..., reason = ...}
>
> Where type and code are optional. If we go for any other option,
> we will need to split it in 2 versions: either
>
> {type = ..., reason = ...}
>
> or
>
> {code = ..., reason = ...}
>
> One can argue, that error codes can intersect, but the thing
> is that ClientError and CustomError are different types for a user.
> box.error.* codes are valid only and only for ClientError objects.
>
> Moreover, even now I can specify not existing codes when create a
> ClientError object. So intersection argument does not work - codes
> can intersect even now. And it seems to be not a problem.
>
> Just consider this example:
>
> tarantool> args = {code = 214, reason = 'Message'}
> ---
> ...
>
> tarantool> err = box.error.new(args)
> ---
> ...
>
> tarantool> err:unpack()
> ---
> - code: 214
> base_type: ClientError
> type: ClientError
> message: Message
> trace:
> - file: '[string "err = box.error.new(args)"]'
> line: 1
> ...
>
> I created a ClientError with code 214. Not CustomError. ClientError.
> It means, that code is never going to be enough to detect error type.
> You will never be able to compare code with 214, and if they are
> equal, say that it is CustomError. The example above proves it is not
> true. You always need to look at error type first. Therefore it makes
> no sense to ban codes for CustomErrors. It won't protect from anything.
> Updated, now the CustomError code is independent of the ClientError code.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Tarantool-patches] [PATCH V4 2/6] error: send custom type in IProto
2020-04-16 17:38 [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Leonid Vasiliev
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 1/6] error: add custom error type Leonid Vasiliev
@ 2020-04-16 17:38 ` Leonid Vasiliev
2020-04-17 0:51 ` Vladislav Shpilevoy
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 3/6] session: add offset to SQL session settings array Leonid Vasiliev
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Leonid Vasiliev @ 2020-04-16 17:38 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Error custom type features were added to the public
Lua API in the previous commits. This one makes the new attribute
being sent in IProto.
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
Needed for #4398
@TarantoolBot document
Title: New error object attributes in IProto
Error objects in IProto already have 2 fields:
`IPROTO_ERROR_CODE = 0x01` and `IPROTO_ERROR_MESSAGE = 0x02`.
Now added:
`IPROTO_ERROR_CUSTOM_TYPE = 0x03`.
It's optional, have MP_STR type, and speak for themselves.
Custom error type is error object attribute.
This is what a user specifies in
`box.error.new({type = <custom_type>})` or
`box.error.new(<custom_type>)`.
---
| 1 +
src/box/iproto_constants.h | 1 +
src/box/lua/net_box.lua | 14 +++++++++++---
src/box/xrow.c | 9 ++++++++-
test/box/error.result | 40 ++++++++++++++++++++++++++++++++++++++++
test/box/error.test.lua | 17 +++++++++++++++++
6 files changed, 78 insertions(+), 4 deletions(-)
--git a/extra/exports b/extra/exports
index 9467398..b01b437 100644
--- a/extra/exports
+++ b/extra/exports
@@ -242,6 +242,7 @@ box_error_last
box_error_clear
box_error_set
error_set_prev
+error_set_traceback
box_latch_new
box_latch_delete
box_latch_lock
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 7ed8296..1fca97b 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -154,6 +154,7 @@ enum iproto_ballot_key {
enum iproto_error_key {
IPROTO_ERROR_CODE = 0x01,
IPROTO_ERROR_MESSAGE = 0x02,
+ IPROTO_ERROR_CUSTOM_TYPE = 0x03,
};
#define bit(c) (1ULL<<IPROTO_##c)
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 07fa54c..95f9cf8 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -10,6 +10,11 @@ local urilib = require('uri')
local internal = require('net.box.lib')
local trigger = require('internal.trigger')
+ffi.cdef[[
+void
+error_set_traceback(struct error *e, const char *traceback);
+]]
+
local band = bit.band
local max = math.max
local fiber_clock = fiber.clock
@@ -47,6 +52,7 @@ local IPROTO_ERROR_KEY = 0x31
local IPROTO_ERROR_STACK = 0x52
local IPROTO_ERROR_CODE = 0x01
local IPROTO_ERROR_MESSAGE = 0x02
+local IPROTO_ERROR_CUSTOM_TYPE = 0x03
local IPROTO_GREETING_SIZE = 128
local IPROTO_CHUNK_KEY = 128
local IPROTO_OK_KEY = 0
@@ -285,9 +291,11 @@ local function create_transport(host, port, user, password, callback,
local prev = nil
for i = #self.response, 1, -1 do
local error = self.response[i]
- local code = error[IPROTO_ERROR_CODE]
- local msg = error[IPROTO_ERROR_MESSAGE]
- local new_err = box.error.new({code = code, reason = msg})
+ local new_err = box.error.new({
+ type = error[IPROTO_ERROR_CUSTOM_TYPE],
+ code = error[IPROTO_ERROR_CODE],
+ reason = error[IPROTO_ERROR_MESSAGE],
+ })
new_err:set_prev(prev)
prev = new_err
end
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 5494b41..a717da6 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -494,11 +494,18 @@ mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
mpstream_encode_uint(stream, IPROTO_ERROR_STACK);
mpstream_encode_array(stream, err_cnt);
for (const struct error *it = error; it != NULL; it = it->cause) {
- mpstream_encode_map(stream, 2);
+ uint32_t map_size = 2;
+ const char *custom_type = box_error_custom_type(it);
+ map_size += (custom_type != NULL);
+ mpstream_encode_map(stream, map_size);
mpstream_encode_uint(stream, IPROTO_ERROR_CODE);
mpstream_encode_uint(stream, box_error_code(it));
mpstream_encode_uint(stream, IPROTO_ERROR_MESSAGE);
mpstream_encode_str(stream, it->errmsg);
+ if (custom_type != NULL) {
+ mpstream_encode_uint(stream, IPROTO_ERROR_CUSTOM_TYPE);
+ mpstream_encode_str(stream, custom_type);
+ }
}
}
diff --git a/test/box/error.result b/test/box/error.result
index 82db453..d231f5d 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -889,3 +889,43 @@ e = box.error.new({type = string.rep('a', 128)})
| ---
| - 63
| ...
+
+--
+-- Check how custom error type are passed through
+-- IProto.
+--
+netbox = require('net.box')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'super')
+ | ---
+ | ...
+c = netbox.connect(box.cfg.listen)
+ | ---
+ | ...
+_, e = pcall(c.call, c, 'box.error', { \
+ {code = 123, type = 'TestType', reason = 'Test reason'} \
+})
+ | ---
+ | ...
+e = e:unpack()
+ | ---
+ | ...
+e.trace = nil
+ | ---
+ | ...
+e
+ | ---
+ | - code: 214
+ | base_type: CustomError
+ | type: TestType
+ | custom_type: TestType
+ | message: Test reason
+ | ...
+
+c:close()
+ | ---
+ | ...
+box.schema.user.revoke('guest', 'super')
+ | ---
+ | ...
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index 38f7406..3258024 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -244,3 +244,20 @@ e:unpack()
-- Try too long type name.
e = box.error.new({type = string.rep('a', 128)})
#e.type
+
+--
+-- Check how custom error type are passed through
+-- IProto.
+--
+netbox = require('net.box')
+box.schema.user.grant('guest', 'super')
+c = netbox.connect(box.cfg.listen)
+_, e = pcall(c.call, c, 'box.error', { \
+ {code = 123, type = 'TestType', reason = 'Test reason'} \
+})
+e = e:unpack()
+e.trace = nil
+e
+
+c:close()
+box.schema.user.revoke('guest', 'super')
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Tarantool-patches] [PATCH V4 2/6] error: send custom type in IProto
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 2/6] error: send custom type in IProto Leonid Vasiliev
@ 2020-04-17 0:51 ` Vladislav Shpilevoy
0 siblings, 0 replies; 16+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-17 0:51 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches
Thanks for the patch!
The commit message still contains plural verbs and nouns.
I changed it to:
====================
Error custom type feature was added to the public Lua API in the
previous commit. This one makes the new attribute being sent in
IProto.
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
Needed for #4398
@TarantoolBot document
Title: New error object attribute in IProto
Error objects in IProto already have 2 fields:
`IPROTO_ERROR_CODE = 0x01` and `IPROTO_ERROR_MESSAGE = 0x02`.
Now added:
`IPROTO_ERROR_CUSTOM_TYPE = 0x03`.
It's optional, has MP_STR type, and speaks for itself.
Custom error type is error object attribute.
This is what a user specifies in
`box.error.new({type = <custom_type>})` or
`box.error.new(<custom_type>)`.
====================
See 5 comments below. Some of them I fixed by force push, but
forgot to extract diff first, sorry.
On 16/04/2020 19:38, Leonid Vasiliev wrote:
> Error custom type features were added to the public
> Lua API in the previous commits. This one makes the new attribute
> being sent in IProto.
>
> Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
>
> Needed for #4398
>
> @TarantoolBot document
> Title: New error object attributes in IProto
>
> Error objects in IProto already have 2 fields:
> `IPROTO_ERROR_CODE = 0x01` and `IPROTO_ERROR_MESSAGE = 0x02`.
>
> Now added:
>
> `IPROTO_ERROR_CUSTOM_TYPE = 0x03`.
>
> It's optional, have MP_STR type, and speak for themselves.
> Custom error type is error object attribute.
> This is what a user specifies in
> `box.error.new({type = <custom_type>})` or
> `box.error.new(<custom_type>)`.
> ---
> extra/exports | 1 +
> src/box/iproto_constants.h | 1 +
> src/box/lua/net_box.lua | 14 +++++++++++---
> src/box/xrow.c | 9 ++++++++-
> test/box/error.result | 40 ++++++++++++++++++++++++++++++++++++++++
> test/box/error.test.lua | 17 +++++++++++++++++
> 6 files changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/extra/exports b/extra/exports
> index 9467398..b01b437 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -242,6 +242,7 @@ box_error_last
> box_error_clear
> box_error_set
> error_set_prev
> +error_set_traceback
1. Garbage left from remove traceback patch.
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 07fa54c..95f9cf8 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -10,6 +10,11 @@ local urilib = require('uri')
> local internal = require('net.box.lib')
> local trigger = require('internal.trigger')
>
> +ffi.cdef[[
> +void
> +error_set_traceback(struct error *e, const char *traceback);
> +]]
2. The same.
> diff --git a/test/box/error.result b/test/box/error.result
> index 82db453..d231f5d 100644
> --- a/test/box/error.result
> +++ b/test/box/error.result
> @@ -889,3 +889,43 @@ e = box.error.new({type = string.rep('a', 128)})
> | ---
> | - 63
> | ...
> +
> +--
> +-- Check how custom error type are passed through
3. Still uses plural.
> +-- IProto.
> +--
> +netbox = require('net.box')
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'super')
> + | ---
> + | ...
> +c = netbox.connect(box.cfg.listen)
> + | ---
> + | ...
> +_, e = pcall(c.call, c, 'box.error', { \
> + {code = 123, type = 'TestType', reason = 'Test reason'} \
4. '\' symbols were aligned in my version of the patch.
> +})
> + | ---
> + | ...
> +e = e:unpack()
> + | ---
> + | ...
> +e.trace = nil
> + | ---
> + | ...
> +e
> + | ---
> + | - code: 214
5. The code was '123' in the call above, but here it is 214. The
code was silently ignored. This is what I am talking about in the
previous commit's discussion.
> + | base_type: CustomError
> + | type: TestType
> + | custom_type: TestType
> + | message: Test reason
> + | ...
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Tarantool-patches] [PATCH V4 3/6] session: add offset to SQL session settings array
2020-04-16 17:38 [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Leonid Vasiliev
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 1/6] error: add custom error type Leonid Vasiliev
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 2/6] error: send custom type in IProto Leonid Vasiliev
@ 2020-04-16 17:38 ` Leonid Vasiliev
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 4/6] error: add session setting for error type marshaling Leonid Vasiliev
` (3 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Leonid Vasiliev @ 2020-04-16 17:38 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Session settings are stored in a monolithic array. Submodules
can define a range of settings in there. For example, SQL. It
occupies settings from 0 to 8. There is a second array of only
SQL settings in build.c, of the same size, and it uses the same
indexes.
But if something will be added before SQL settings, so as they
won't start from 0, it will break the equal indexes assumption.
SQL should normalize all setting identifiers by
SESSION_SETTING_SQL_BEGIN.
---
src/box/sql/build.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index aecba41..b1f9fed 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3367,7 +3367,8 @@ sql_session_setting_get(int id, const char **mp_pair, const char **mp_pair_end)
assert(id >= SESSION_SETTING_SQL_BEGIN && id < SESSION_SETTING_SQL_END);
struct session *session = current_session();
uint32_t flags = session->sql_flags;
- struct sql_option_metadata *opt = &sql_session_opts[id];
+ struct sql_option_metadata *opt =
+ &sql_session_opts[id - SESSION_SETTING_SQL_BEGIN];
uint32_t mask = opt->mask;
const char *name = session_setting_strs[id];
size_t name_len = strlen(name);
@@ -3404,7 +3405,8 @@ static int
sql_set_boolean_option(int id, bool value)
{
struct session *session = current_session();
- struct sql_option_metadata *option = &sql_session_opts[id];
+ struct sql_option_metadata *option =
+ &sql_session_opts[id - SESSION_SETTING_SQL_BEGIN];
assert(option->field_type == FIELD_TYPE_BOOLEAN);
#ifdef NDEBUG
if ((session->sql_flags & SQL_SqlTrace) == 0) {
@@ -3431,7 +3433,8 @@ sql_set_boolean_option(int id, bool value)
static int
sql_set_string_option(int id, const char *value)
{
- assert(sql_session_opts[id].field_type = FIELD_TYPE_STRING);
+ assert(sql_session_opts[id - SESSION_SETTING_SQL_BEGIN].field_type =
+ FIELD_TYPE_STRING);
assert(id == SESSION_SETTING_SQL_DEFAULT_ENGINE);
(void)id;
enum sql_storage_engine engine = STR2ENUM(sql_storage_engine, value);
@@ -3448,7 +3451,8 @@ sql_session_setting_set(int id, const char *mp_value)
{
assert(id >= SESSION_SETTING_SQL_BEGIN && id < SESSION_SETTING_SQL_END);
enum mp_type mtype = mp_typeof(*mp_value);
- enum field_type stype = sql_session_opts[id].field_type;
+ enum field_type stype =
+ sql_session_opts[id - SESSION_SETTING_SQL_BEGIN].field_type;
uint32_t len;
const char *tmp;
switch(stype) {
@@ -3473,10 +3477,10 @@ sql_session_setting_set(int id, const char *mp_value)
void
sql_session_settings_init()
{
- for (int id = SESSION_SETTING_SQL_BEGIN; id < SESSION_SETTING_SQL_END;
- ++id) {
+ for (int i = 0, id = SESSION_SETTING_SQL_BEGIN;
+ id < SESSION_SETTING_SQL_END; ++id, ++i) {
struct session_setting *setting = &session_settings[id];
- setting->field_type = sql_session_opts[id].field_type;
+ setting->field_type = sql_session_opts[i].field_type;
setting->get = sql_session_setting_get;
setting->set = sql_session_setting_set;
}
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread* [Tarantool-patches] [PATCH V4 4/6] error: add session setting for error type marshaling
2020-04-16 17:38 [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Leonid Vasiliev
` (2 preceding siblings ...)
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 3/6] session: add offset to SQL session settings array Leonid Vasiliev
@ 2020-04-16 17:38 ` Leonid Vasiliev
2020-04-16 19:48 ` Konstantin Osipov
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 5/6] error: update constructors of some errors Leonid Vasiliev
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Leonid Vasiliev @ 2020-04-16 17:38 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Errors are encoded as a string when serialized to MessagePack to
be sent over IProto or when just saved into a buffer via Lua
modules msgpackffi and msgpack.
That is not very useful on client-side, because most of the error
metadata is lost: code, type, trace - everything except the
message.
Next commits are going to dedicate a new MP_EXT type to error
objects so as everything could be encoded, and on client side it
would be possible to restore types.
But this is a breaking change in case some users use old
connectors when work with newer Tarantool instances. So to smooth
the upgrade there is a new session setting -
'error_marshaling_enabled'.
By default it is false. When it is true, all fibers of the given
session will serialize error objects as MP_EXT.
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
Needed for #4398
---
src/box/lua/call.c | 33 ++++++++++++++---------
src/box/lua/execute.c | 2 +-
src/box/lua/tuple.c | 12 ++++-----
src/box/session.cc | 5 +++-
src/box/session.h | 5 ++--
src/box/session_settings.c | 56 ++++++++++++++++++++++++++++++++++++++++
src/box/session_settings.h | 1 +
src/box/sql/func.c | 4 ++-
src/lua/msgpack.c | 25 +++++++++---------
src/lua/msgpack.h | 8 +++---
src/lua/utils.c | 11 +++++---
src/lua/utils.h | 8 ++++--
src/serializer_opts.h | 44 +++++++++++++++++++++++++++++++
test/box/session_settings.result | 3 ++-
14 files changed, 172 insertions(+), 45 deletions(-)
create mode 100644 src/serializer_opts.h
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 1d29494..91e291c 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -46,6 +46,7 @@
#include "small/obuf.h"
#include "trivia/util.h"
#include "mpstream/mpstream.h"
+#include "box/session.h"
/**
* A helper to find a Lua function by name and put it
@@ -174,7 +175,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
*/
for (int i = 1; i <= nrets; ++i) {
struct luaL_field field;
- if (luaL_tofield(L, cfg, i, &field) < 0)
+ if (luaL_tofield(L, cfg, NULL, i, &field) < 0)
return luaT_error(L);
struct tuple *tuple;
if (field.type == MP_EXT &&
@@ -188,11 +189,11 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
*/
lua_pushvalue(L, i);
mpstream_encode_array(stream, 1);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
} else {
/* `return ..., array, ...` */
- luamp_encode(L, cfg, stream, i);
+ luamp_encode(L, cfg, NULL, stream, i);
}
}
return nrets;
@@ -203,7 +204,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
* Inspect the first result
*/
struct luaL_field root;
- if (luaL_tofield(L, cfg, 1, &root) < 0)
+ if (luaL_tofield(L, cfg, NULL, 1, &root) < 0)
return luaT_error(L);
struct tuple *tuple;
if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
@@ -217,7 +218,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
*/
mpstream_encode_array(stream, 1);
assert(lua_gettop(L) == 1);
- luamp_encode_r(L, cfg, stream, &root, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &root, 0);
return 1;
}
@@ -233,7 +234,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
for (uint32_t t = 1; t <= root.size; t++) {
lua_rawgeti(L, 1, t);
struct luaL_field field;
- if (luaL_tofield(L, cfg, -1, &field) < 0)
+ if (luaL_tofield(L, cfg, NULL, -1, &field) < 0)
return luaT_error(L);
if (field.type == MP_EXT && (tuple = luaT_istuple(L, -1))) {
tuple_to_mpstream(tuple, stream);
@@ -249,13 +250,13 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
* Encode the first field of tuple using
* existing information from luaL_tofield
*/
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
assert(lua_gettop(L) == 1);
/* Encode remaining fields as usual */
for (uint32_t f = 2; f <= root.size; f++) {
lua_rawgeti(L, 1, f);
- luamp_encode(L, cfg, stream, -1);
+ luamp_encode(L, cfg, NULL, stream, -1);
lua_pop(L, 1);
}
return 1;
@@ -265,10 +266,10 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
* { tuple/array, ..., { scalar }, ... }`
*/
mpstream_encode_array(stream, 1);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
} else {
/* `return { tuple/array, ..., tuple/array, ... }` */
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
}
lua_pop(L, 1);
assert(lua_gettop(L) == 1);
@@ -379,6 +380,11 @@ execute_lua_eval(lua_State *L)
struct encode_lua_ctx {
struct port_lua *port;
struct mpstream *stream;
+ /*
+ * Lua serializer additional options.
+ * Used for sets the session specific serialization options.
+ */
+ struct serializer_opts *serializer_opts;
};
static int
@@ -393,8 +399,10 @@ encode_lua_call(lua_State *L)
*/
struct luaL_serializer *cfg = luaL_msgpack_default;
int size = lua_gettop(ctx->port->L);
- for (int i = 1; i <= size; ++i)
- luamp_encode(ctx->port->L, cfg, ctx->stream, i);
+ for (int i = 1; i <= size; ++i) {
+ luamp_encode(ctx->port->L, cfg, ctx->serializer_opts,
+ ctx->stream, i);
+ }
ctx->port->size = size;
mpstream_flush(ctx->stream);
return 0;
@@ -429,6 +437,7 @@ port_lua_do_dump(struct port *base, struct mpstream *stream,
struct encode_lua_ctx ctx;
ctx.port = port;
ctx.stream = stream;
+ ctx.serializer_opts = ¤t_session()->meta.serializer_opts;
struct lua_State *L = tarantool_L;
int top = lua_gettop(L);
if (lua_cpcall(L, handler, &ctx) != 0) {
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index b4843b2..b5db3c9 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -328,7 +328,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
bind->name = NULL;
bind->name_len = 0;
}
- if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0)
+ if (luaL_tofield(L, luaL_msgpack_default, NULL, -1, &field) < 0)
return -1;
switch (field.type) {
case MP_UINT:
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 4b0701e..aba906d 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -120,7 +120,7 @@ luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
int argc = lua_gettop(L);
mpstream_encode_array(&stream, argc);
for (int k = 1; k <= argc; ++k) {
- luamp_encode(L, luaL_msgpack_default, &stream, k);
+ luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
}
} else {
/* Create the tuple from a Lua table. */
@@ -252,18 +252,18 @@ luamp_convert_key(struct lua_State *L, struct luaL_serializer *cfg,
return tuple_to_mpstream(tuple, stream);
struct luaL_field field;
- if (luaL_tofield(L, cfg, index, &field) < 0)
+ if (luaL_tofield(L, cfg, NULL, index, &field) < 0)
luaT_error(L);
if (field.type == MP_ARRAY) {
lua_pushvalue(L, index);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
} else if (field.type == MP_NIL) {
mpstream_encode_array(stream, 0);
} else {
mpstream_encode_array(stream, 1);
lua_pushvalue(L, index);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
}
}
@@ -275,7 +275,7 @@ luamp_encode_tuple(struct lua_State *L, struct luaL_serializer *cfg,
struct tuple *tuple = luaT_istuple(L, index);
if (tuple != NULL) {
return tuple_to_mpstream(tuple, stream);
- } else if (luamp_encode(L, cfg, stream, index) != MP_ARRAY) {
+ } else if (luamp_encode(L, cfg, NULL, stream, index) != MP_ARRAY) {
diag_set(ClientError, ER_TUPLE_NOT_ARRAY);
luaT_error(L);
}
@@ -435,7 +435,7 @@ lbox_tuple_transform(struct lua_State *L)
mpstream_encode_array(&stream, 3);
mpstream_encode_str(&stream, "!");
mpstream_encode_uint(&stream, offset);
- luamp_encode(L, luaL_msgpack_default, &stream, i);
+ luamp_encode(L, luaL_msgpack_default, NULL, &stream, i);
}
mpstream_flush(&stream);
diff --git a/src/box/session.cc b/src/box/session.cc
index b557eed..08a1092 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -275,6 +275,9 @@ session_find(uint64_t sid)
mh_i64ptr_node(session_registry, k)->val;
}
+extern "C" void
+session_settings_init(void);
+
void
session_init()
{
@@ -283,7 +286,7 @@ session_init()
panic("out of memory");
mempool_create(&session_pool, &cord()->slabc, sizeof(struct session));
credentials_create(&admin_credentials, admin_user);
- sql_session_settings_init();
+ session_settings_init();
}
void
diff --git a/src/box/session.h b/src/box/session.h
index 8693799..500a88b 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -36,13 +36,12 @@
#include "fiber.h"
#include "user.h"
#include "authentication.h"
+#include "serializer_opts.h"
#if defined(__cplusplus)
extern "C" {
#endif /* defined(__cplusplus) */
-extern void sql_session_settings_init();
-
struct port;
struct session_vtab;
@@ -90,6 +89,8 @@ struct session_meta {
};
/** Console output format. */
enum output_format output_format;
+ /** Session-specific serialization options. */
+ struct serializer_opts serializer_opts;
};
/**
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 79c4b8d..dbbbf24 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -42,6 +42,7 @@ struct session_setting session_settings[SESSION_SETTING_COUNT] = {};
/** Corresponding names of session settings. */
const char *session_setting_strs[SESSION_SETTING_COUNT] = {
+ "error_marshaling_enabled",
"sql_default_engine",
"sql_defer_foreign_keys",
"sql_full_column_names",
@@ -449,3 +450,58 @@ session_setting_find(const char *name) {
else
return -1;
}
+
+/* Module independent session settings. */
+
+static void
+session_setting_error_marshaling_enabled_get(int id, const char **mp_pair,
+ const char **mp_pair_end)
+{
+ assert(id == SESSION_SETTING_ERROR_MARSHALING_ENABLED);
+ struct session *session = current_session();
+ const char *name = session_setting_strs[id];
+ size_t name_len = strlen(name);
+ bool value = session->meta.serializer_opts.error_marshaling_enabled;
+ size_t size = mp_sizeof_array(2) + mp_sizeof_str(name_len) +
+ mp_sizeof_bool(value);
+
+ char *pos = (char*)static_alloc(size);
+ assert(pos != NULL);
+ char *pos_end = mp_encode_array(pos, 2);
+ pos_end = mp_encode_str(pos_end, name, name_len);
+ pos_end = mp_encode_bool(pos_end, value);
+ *mp_pair = pos;
+ *mp_pair_end = pos_end;
+}
+
+static int
+session_setting_error_marshaling_enabled_set(int id, const char *mp_value)
+{
+ assert(id == SESSION_SETTING_ERROR_MARSHALING_ENABLED);
+ enum mp_type mtype = mp_typeof(*mp_value);
+ enum field_type stype = session_settings[id].field_type;
+ if (mtype != MP_BOOL) {
+ diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+ session_setting_strs[id], field_type_strs[stype]);
+ return -1;
+ }
+ struct session *session = current_session();
+ session->meta.serializer_opts.error_marshaling_enabled =
+ mp_decode_bool(&mp_value);
+ return 0;
+}
+
+extern void
+sql_session_settings_init();
+
+void
+session_settings_init(void)
+{
+ struct session_setting *s =
+ &session_settings[SESSION_SETTING_ERROR_MARSHALING_ENABLED];
+ s->field_type = FIELD_TYPE_BOOLEAN;
+ s->get = session_setting_error_marshaling_enabled_get;
+ s->set = session_setting_error_marshaling_enabled_set;
+
+ sql_session_settings_init();
+}
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index e2adc52..6560b8c 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -42,6 +42,7 @@
* space iterator will not be sorted properly.
*/
enum {
+ SESSION_SETTING_ERROR_MARSHALING_ENABLED,
SESSION_SETTING_SQL_BEGIN,
SESSION_SETTING_SQL_DEFAULT_ENGINE = SESSION_SETTING_SQL_BEGIN,
SESSION_SETTING_SQL_DEFER_FOREIGN_KEYS,
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index e3a6e88..3768372 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -257,8 +257,10 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
return NULL;
for (int i = 0; i < argc; i++) {
struct luaL_field field;
- if (luaL_tofield(L, luaL_msgpack_default, -1 - i, &field) < 0)
+ if (luaL_tofield(L, luaL_msgpack_default,
+ NULL, -1 - i, &field) < 0) {
goto error;
+ }
switch (field.type) {
case MP_BOOL:
mem_set_bool(&val[i], field.bval);
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index e4fb0cf..b14361f 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -109,8 +109,8 @@ luamp_set_decode_extension(luamp_decode_extension_f handler)
enum mp_type
luamp_encode_r(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, struct luaL_field *field,
- int level)
+ struct serializer_opts *opts, struct mpstream *stream,
+ struct luaL_field *field, int level)
{
int top = lua_gettop(L);
enum mp_type type;
@@ -155,13 +155,13 @@ restart: /* used by MP_EXT of unidentified subtype */
lua_pushnil(L); /* first key */
while (lua_next(L, top) != 0) {
lua_pushvalue(L, -2); /* push a copy of key to top */
- if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+ if (luaL_tofield(L, cfg, opts, lua_gettop(L), field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, opts, stream, field, level + 1);
lua_pop(L, 1); /* pop a copy of key */
- if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+ if (luaL_tofield(L, cfg, opts, lua_gettop(L), field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, opts, stream, field, level + 1);
lua_pop(L, 1); /* pop value */
}
assert(lua_gettop(L) == top);
@@ -180,9 +180,9 @@ restart: /* used by MP_EXT of unidentified subtype */
mpstream_encode_array(stream, size);
for (uint32_t i = 0; i < size; i++) {
lua_rawgeti(L, top, i + 1);
- if (luaL_tofield(L, cfg, top + 1, field) < 0)
+ if (luaL_tofield(L, cfg, opts, top + 1, field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, opts, stream, field, level + 1);
lua_pop(L, 1);
}
assert(lua_gettop(L) == top);
@@ -213,7 +213,8 @@ restart: /* used by MP_EXT of unidentified subtype */
enum mp_type
luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, int index)
+ struct serializer_opts *opts, struct mpstream *stream,
+ int index)
{
int top = lua_gettop(L);
if (index < 0)
@@ -225,9 +226,9 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
}
struct luaL_field field;
- if (luaL_tofield(L, cfg, lua_gettop(L), &field) < 0)
+ if (luaL_tofield(L, cfg, opts, lua_gettop(L), &field) < 0)
return luaT_error(L);
- enum mp_type top_type = luamp_encode_r(L, cfg, stream, &field, 0);
+ enum mp_type top_type = luamp_encode_r(L, cfg, opts, stream, &field, 0);
if (!on_top) {
lua_remove(L, top + 1); /* remove a value copy */
@@ -369,7 +370,7 @@ lua_msgpack_encode(lua_State *L)
mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
luamp_error, L);
- luamp_encode(L, cfg, &stream, 1);
+ luamp_encode(L, cfg, NULL, &stream, 1);
mpstream_flush(&stream);
if (index > 1) {
diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h
index d0ade30..279e056 100644
--- a/src/lua/msgpack.h
+++ b/src/lua/msgpack.h
@@ -43,6 +43,7 @@ extern "C" {
struct luaL_serializer;
struct mpstream;
+struct serializer_opts;
/**
* Default instance of msgpack serializer (msgpack = require('msgpack')).
@@ -63,12 +64,13 @@ enum { LUAMP_ALLOC_FACTOR = 256 };
/* low-level function needed for execute_lua_call() */
enum mp_type
luamp_encode_r(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, struct luaL_field *field,
- int level);
+ struct serializer_opts *opts, struct mpstream *stream,
+ struct luaL_field *field, int level);
enum mp_type
luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, int index);
+ struct serializer_opts *opts, struct mpstream *stream,
+ int index);
void
luamp_decode(struct lua_State *L, struct luaL_serializer *cfg,
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 667365f..bd320f3 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -452,7 +452,7 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
lua_pcall(L, 1, 1, 0);
/* replace obj with the unpacked value */
lua_replace(L, idx);
- if (luaL_tofield(L, cfg, idx, field) < 0)
+ if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
luaT_error(L);
} /* else ignore lua_gettable exceptions */
lua_settop(L, top); /* remove temporary objects */
@@ -515,7 +515,7 @@ lua_field_try_serialize(struct lua_State *L)
/* copy object itself */
lua_pushvalue(L, 1);
lua_call(L, 1, 1);
- s->is_error = (luaL_tofield(L, cfg, -1, field) != 0);
+ s->is_error = (luaL_tofield(L, cfg, NULL, -1, field) != 0);
s->is_value_returned = true;
return 1;
}
@@ -641,14 +641,17 @@ lua_field_tostring(struct lua_State *L, struct luaL_serializer *cfg, int idx,
lua_call(L, 1, 1);
lua_replace(L, idx);
lua_settop(L, top);
- if (luaL_tofield(L, cfg, idx, field) < 0)
+ if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
luaT_error(L);
}
int
-luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
+luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
+ struct serializer_opts *opts, int index,
struct luaL_field *field)
{
+ /* opts will be used for encode MP_ERROR in the future */
+ (void)opts;
if (index < 0)
index = lua_gettop(L) + index + 1;
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 4bc0417..cf9ef76 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -59,6 +59,8 @@ extern "C" {
#include "lib/core/mp_extension_types.h"
#include "lib/core/decimal.h" /* decimal_t */
+#include "serializer_opts.h"
+
struct lua_State;
struct ibuf;
struct tt_uuid;
@@ -366,6 +368,7 @@ struct luaL_field {
*
* @param L stack
* @param cfg configuration
+ * @param serializer_opts the Lua serializer additional options
* @param index stack index
* @param field conversion result
*
@@ -373,7 +376,8 @@ struct luaL_field {
* @retval -1 Error.
*/
int
-luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
+luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
+ struct serializer_opts *opts, int index,
struct luaL_field *field);
/**
@@ -412,7 +416,7 @@ static inline void
luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
struct luaL_field *field)
{
- if (luaL_tofield(L, cfg, idx, field) < 0)
+ if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
luaT_error(L);
if (field->type != MP_EXT || field->ext_type != MP_UNKNOWN_EXTENSION)
return;
diff --git a/src/serializer_opts.h b/src/serializer_opts.h
new file mode 100644
index 0000000..9e2c15e
--- /dev/null
+++ b/src/serializer_opts.h
@@ -0,0 +1,44 @@
+#pragma once
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/**
+ * Serializer options which can regulate how to serialize
+ * something in scope of one session.
+ */
+struct serializer_opts {
+ /**
+ * When enabled, error objects get their own MP_EXT
+ * MessagePack type and therefore can be type-safely
+ * transmitted over the network.
+ */
+ bool error_marshaling_enabled;
+};
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index ea6302d..149cc4b 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -52,7 +52,8 @@ s:replace({'sql_defer_foreign_keys', true})
--
s:select()
| ---
- | - - ['sql_default_engine', 'memtx']
+ | - - ['error_marshaling_enabled', false]
+ | - ['sql_default_engine', 'memtx']
| - ['sql_defer_foreign_keys', false]
| - ['sql_full_column_names', false]
| - ['sql_full_metadata', false]
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Tarantool-patches] [PATCH V4 4/6] error: add session setting for error type marshaling
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 4/6] error: add session setting for error type marshaling Leonid Vasiliev
@ 2020-04-16 19:48 ` Konstantin Osipov
2020-04-17 0:51 ` Vladislav Shpilevoy
0 siblings, 1 reply; 16+ messages in thread
From: Konstantin Osipov @ 2020-04-16 19:48 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches, v.shpilevoy
* Leonid Vasiliev <lvasiliev@tarantool.org> [20/04/16 20:45]:
> Errors are encoded as a string when serialized to MessagePack to
> be sent over IProto or when just saved into a buffer via Lua
> modules msgpackffi and msgpack.
>
> That is not very useful on client-side, because most of the error
> metadata is lost: code, type, trace - everything except the
> message.
>
> Next commits are going to dedicate a new MP_EXT type to error
> objects so as everything could be encoded, and on client side it
> would be possible to restore types.
>
> But this is a breaking change in case some users use old
> connectors when work with newer Tarantool instances. So to smooth
> the upgrade there is a new session setting -
> 'error_marshaling_enabled'.
>
> By default it is false. When it is true, all fibers of the given
> session will serialize error objects as MP_EXT.
I don't understand the point of having an MP_EXT if the same
can be achieved with simply a new messagepack key. There is
already IPROTO_ERROR_CODE, IPROTO_ERROR_MESSAGE, why can't you add
IPROTO_ERROR_TYPE?
And since this is a protocol change, there should have been (a
short?) RFC for it.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Tarantool-patches] [PATCH V4 4/6] error: add session setting for error type marshaling
2020-04-16 19:48 ` Konstantin Osipov
@ 2020-04-17 0:51 ` Vladislav Shpilevoy
2020-04-17 7:35 ` Konstantin Osipov
0 siblings, 1 reply; 16+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-17 0:51 UTC (permalink / raw)
To: Konstantin Osipov, Leonid Vasiliev, tarantool-patches
Hi!
On 16/04/2020 21:48, Konstantin Osipov wrote:
> * Leonid Vasiliev <lvasiliev@tarantool.org> [20/04/16 20:45]:
>> Errors are encoded as a string when serialized to MessagePack to
>> be sent over IProto or when just saved into a buffer via Lua
>> modules msgpackffi and msgpack.
>>
>> That is not very useful on client-side, because most of the error
>> metadata is lost: code, type, trace - everything except the
>> message.
>>
>> Next commits are going to dedicate a new MP_EXT type to error
>> objects so as everything could be encoded, and on client side it
>> would be possible to restore types.
>>
>> But this is a breaking change in case some users use old
>> connectors when work with newer Tarantool instances. So to smooth
>> the upgrade there is a new session setting -
>> 'error_marshaling_enabled'.
>>
>> By default it is false. When it is true, all fibers of the given
>> session will serialize error objects as MP_EXT.
>
> I don't understand the point of having an MP_EXT if the same
> can be achieved with simply a new messagepack key. There is
> already IPROTO_ERROR_CODE, IPROTO_ERROR_MESSAGE, why can't you add
> IPROTO_ERROR_TYPE?
Seems like you are talking about the case when error object is
thrown as an exception. But the patchset is about being able to
recognize and decode errors even when they are returned inside
response body, as a part of IPROTO_OK response type.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Tarantool-patches] [PATCH V4 4/6] error: add session setting for error type marshaling
2020-04-17 0:51 ` Vladislav Shpilevoy
@ 2020-04-17 7:35 ` Konstantin Osipov
0 siblings, 0 replies; 16+ messages in thread
From: Konstantin Osipov @ 2020-04-17 7:35 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/04/17 10:16]:
> Seems like you are talking about the case when error object is
> thrown as an exception. But the patchset is about being able to
> recognize and decode errors even when they are returned inside
> response body, as a part of IPROTO_OK response type.
Then you think this justifies inventing a custom marshalling
scheme and thus forcing all clients to not just patch the driver
but also patch or even re-implement msgpack library they use?
Worse yet, with such a huge protocol change you can add complete
marshalling of any object - SQL parse tree, Lua thread along with
all its referenced objects, Lua code and stack data, and what not
-- all very desired for other features, like distributed SQL or
shipping/executing Lua code close to data.
Worse *yet*, it's quite possible to add all these features without
an MP_EXT. But this doesn't concern you at all.
So what about an RFC?
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Tarantool-patches] [PATCH V4 5/6] error: update constructors of some errors
2020-04-16 17:38 [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Leonid Vasiliev
` (3 preceding siblings ...)
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 4/6] error: add session setting for error type marshaling Leonid Vasiliev
@ 2020-04-16 17:38 ` Leonid Vasiliev
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding Leonid Vasiliev
2020-04-17 0:51 ` [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Vladislav Shpilevoy
6 siblings, 0 replies; 16+ messages in thread
From: Leonid Vasiliev @ 2020-04-16 17:38 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
We want to have a transparent marshalling through net.box
for errors. To do this, we need to recreate the error
on the client side with the same parameters as on the server.
For convenience, we update AccessDeniedError constructor
which has points to static strings and add the XlogGapError
constructor that does not require vclock.
Needed for #4398
---
src/box/error.cc | 18 +++++++++---------
src/box/error.h | 8 ++++++--
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/box/error.cc b/src/box/error.cc
index c8fadfb..cb134ba 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -253,6 +253,13 @@ XlogGapError::XlogGapError(const char *file, unsigned line,
(long long) vclock_sum(to), s_to ? s_to : "");
}
+XlogGapError::XlogGapError(const char *file, unsigned line,
+ const char *msg)
+ : XlogError(&type_XlogGapError, file, line)
+{
+ error_format_msg(this, "%s", msg);
+}
+
struct error *
BuildXlogGapError(const char *file, unsigned line,
const struct vclock *from, const struct vclock *to)
@@ -289,15 +296,8 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
struct on_access_denied_ctx ctx = {access_type, object_type, object_name};
trigger_run(&on_access_denied, (void *) &ctx);
- /*
- * We want to use ctx parameters as error parameters
- * later, so we have to alloc space for it.
- * As m_access_type and m_object_type are constant
- * literals they are statically allocated. We must copy
- * only m_object_name.
- */
- m_object_type = object_type;
- m_access_type = access_type;
+ m_object_type = strdup(object_type);
+ m_access_type = strdup(access_type);
m_object_name = strdup(object_name);
}
diff --git a/src/box/error.h b/src/box/error.h
index 311d543..f70d6cb 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -244,6 +244,8 @@ public:
~AccessDeniedError()
{
free(m_object_name);
+ free(m_object_type);
+ free(m_access_type);
}
const char *
@@ -266,11 +268,11 @@ public:
private:
/** Type of object the required access was denied to */
- const char *m_object_type;
+ char *m_object_type;
/** Name of object the required access was denied to */
char *m_object_name;
/** Type of declined access */
- const char *m_access_type;
+ char *m_access_type;
};
/**
@@ -300,6 +302,8 @@ struct XlogGapError: public XlogError
{
XlogGapError(const char *file, unsigned line,
const struct vclock *from, const struct vclock *to);
+ XlogGapError(const char *file, unsigned line,
+ const char *msg);
virtual void raise() { throw this; }
};
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread* [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding
2020-04-16 17:38 [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Leonid Vasiliev
` (4 preceding siblings ...)
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 5/6] error: update constructors of some errors Leonid Vasiliev
@ 2020-04-16 17:38 ` Leonid Vasiliev
2020-04-17 0:58 ` Vladislav Shpilevoy
2020-04-17 0:51 ` [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Vladislav Shpilevoy
6 siblings, 1 reply; 16+ messages in thread
From: Leonid Vasiliev @ 2020-04-16 17:38 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Closes #4398
@TarantoolBot document
Title: msgpack format for MP_ERROR
For sent an error over IProto MP_ERROR(0x03) subtype
has been added to MsgPack MP_EXT.
Now, if session setting 'error_marshaling_enabled'
is true error will be encoded as:
```
+--------+----------+========+
| MP_EXT | MP_ERROR | MP_MAP |
+--------+----------+========+
```
At the map passes all necessary parameters (depending
from the error type) to create a copy of the error
on the client side.
The necessary fields:
- Error type
- Trace(file name and line)
- Error message
- Errno
Additional fields:
- ClientError code
- Traceback (now only Lua traceback)
- Custom type
- Access Denied object type
- Access Denied object name
- Access Denied access type
---
src/box/CMakeLists.txt | 1 +
src/box/lua/init.c | 56 ++++++
src/box/lua/mp_error.cc | 373 +++++++++++++++++++++++++++++++++++
src/box/lua/mp_error.h | 49 +++++
src/box/lua/tuple.c | 16 --
src/lib/core/mp_extension_types.h | 1 +
src/lua/error.c | 2 -
src/lua/error.h | 3 +-
src/lua/msgpack.c | 3 +
src/lua/utils.c | 6 +-
test/box-tap/extended_error.test.lua | 157 +++++++++++++++
11 files changed, 646 insertions(+), 21 deletions(-)
create mode 100644 src/box/lua/mp_error.cc
create mode 100644 src/box/lua/mp_error.h
create mode 100755 test/box-tap/extended_error.test.lua
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 6688303..7581078 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -151,6 +151,7 @@ add_library(box STATIC
lua/stat.c
lua/ctl.c
lua/error.cc
+ lua/mp_error.cc
lua/session.c
lua/net_box.c
lua/xlog.c
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 63e8b82..de09389 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -36,11 +36,13 @@
#include "lua/utils.h" /* luaT_error() */
#include "lua/trigger.h"
+#include "lua/msgpack.h"
#include "box/box.h"
#include "box/txn.h"
#include "box/func.h"
#include "box/vclock.h"
+#include "box/session.h"
#include "box/lua/error.h"
#include "box/lua/tuple.h"
@@ -62,6 +64,9 @@
#include "box/lua/execute.h"
#include "box/lua/key_def.h"
#include "box/lua/merger.h"
+#include "box/lua/mp_error.h"
+
+#include "mpstream/mpstream.h"
static uint32_t CTID_STRUCT_TXN_SAVEPOINT_PTR = 0;
@@ -386,6 +391,54 @@ static const struct luaL_Reg boxlib_backup[] = {
{NULL, NULL}
};
+/**
+ * A MsgPack extensions handler that supports tuples and errors encode.
+ */
+static enum mp_type
+luamp_encode_extension_box(struct lua_State *L, int idx,
+ struct mpstream *stream)
+{
+ struct tuple *tuple = luaT_istuple(L, idx);
+ if (tuple != NULL) {
+ tuple_to_mpstream(tuple, stream);
+ return MP_ARRAY;
+ }
+
+ if (current_session()->meta.serializer_opts.error_marshaling_enabled) {
+ struct error *err = luaL_iserror(L, idx);
+ if (err != NULL)
+ error_to_mpstream(err, stream);
+ }
+
+ return MP_EXT;
+}
+
+/**
+ * A MsgPack extensions handler that supports errors decode.
+ */
+static void
+luamp_decode_extension_box(struct lua_State *L, const char **data)
+{
+ assert(mp_typeof(**data) == MP_EXT);
+ int8_t ext_type;
+ uint32_t len = mp_decode_extl(data, &ext_type);
+
+ if(ext_type != MP_ERROR) {
+ luaL_error(L, "Unsuported MsgPack extension type: %u",
+ ext_type);
+ return;
+ }
+
+ struct error *err = error_unpack(data, len);
+ if (err == NULL) {
+ luaL_error(L, "Can not parse an error from MsgPack");
+ return;
+ }
+
+ luaT_pusherror(L, err);
+ return;
+}
+
#include "say.h"
void
@@ -426,6 +479,9 @@ box_lua_init(struct lua_State *L)
luaopen_merger(L);
lua_pop(L, 1);
+ luamp_set_encode_extension(luamp_encode_extension_box);
+ luamp_set_decode_extension(luamp_decode_extension_box);
+
/* Load Lua extension */
for (const char **s = lua_sources; *s; s += 2) {
const char *modname = *s;
diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
new file mode 100644
index 0000000..2e15206
--- /dev/null
+++ b/src/box/lua/mp_error.cc
@@ -0,0 +1,373 @@
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include "box/lua/mp_error.h"
+#include "box/error.h"
+#include "mpstream/mpstream.h"
+#include "msgpuck.h"
+#include "mp_extension_types.h"
+
+/**
+ * Keys used for encode/decode MP_ERROR.
+ */
+enum mp_error_details {
+ MP_ERROR_DET_TYPE,
+ MP_ERROR_DET_FILE,
+ MP_ERROR_DET_LINE,
+ MP_ERROR_DET_REASON,
+ MP_ERROR_DET_ERRNO,
+ MP_ERROR_DET_CODE,
+ MP_ERROR_DET_CUSTOM_TYPE,
+ MP_ERROR_DET_AD_OBJ_TYPE,
+ MP_ERROR_DET_AD_OBJ_NAME,
+ MP_ERROR_DET_AD_ACCESS_TYPE
+};
+
+/**
+ * The structure is used for storing parameters
+ * during decoding MP_ERROR.
+ */
+struct mp_error {
+ uint32_t error_code;
+ uint32_t line;
+ uint32_t saved_errno;
+ char *error_type;
+ char *file;
+ char *reason;
+ char *custom_type;
+ char *ad_obj_type;
+ char *ad_obj_name;
+ char *ad_access_type;
+};
+
+static void
+mp_error_init(struct mp_error *mp_error)
+{
+ mp_error->error_code = 0;
+ mp_error->line = 0;
+ mp_error->saved_errno = 0;
+ mp_error->error_type = NULL;
+ mp_error->file = NULL;
+ mp_error->reason = NULL;
+ mp_error->custom_type = NULL;
+ mp_error->ad_obj_type = NULL;
+ mp_error->ad_obj_name = NULL;
+ mp_error->ad_access_type = NULL;
+}
+
+static void
+mp_error_cleanup(struct mp_error *mp_error)
+{
+ free(mp_error->error_type);
+ free(mp_error->file);
+ free(mp_error->reason);
+ free(mp_error->custom_type);
+ free(mp_error->ad_obj_type);
+ free(mp_error->ad_obj_name);
+ free(mp_error->ad_access_type);
+}
+
+void
+error_to_mpstream(struct error *error, struct mpstream *stream)
+{
+ uint32_t errcode = 0;
+ const char *custom_type = NULL;
+ const char *ad_obj_type = NULL;
+ const char *ad_obj_name = NULL;
+ const char *ad_access_type = NULL;
+
+ bool is_client = false;
+ bool is_custom = false;
+ bool is_access_denied = false;
+
+ if (strcmp(error->type->name, "ClientError") == 0) {
+ is_client = true;
+ } else if (strcmp(error->type->name, "CustomError") == 0) {
+ is_custom = true;
+ } else if (strcmp(error->type->name, "AccessDeniedError") == 0) {
+ is_access_denied = true;
+ }
+
+ uint32_t details_num = 5;
+ uint32_t data_size = 0;
+
+ data_size += mp_sizeof_uint(MP_ERROR_DET_TYPE);
+ data_size += mp_sizeof_str(strlen(error->type->name));
+ data_size += mp_sizeof_uint(MP_ERROR_DET_LINE);
+ data_size += mp_sizeof_uint(error->line);
+ data_size += mp_sizeof_uint(MP_ERROR_DET_FILE);
+ data_size += mp_sizeof_str(strlen(error->file));
+ data_size += mp_sizeof_uint(MP_ERROR_DET_REASON);
+ data_size += mp_sizeof_str(strlen(error->errmsg));
+ data_size += mp_sizeof_uint(MP_ERROR_DET_ERRNO);
+ data_size += mp_sizeof_uint(error->saved_errno);
+
+ if (is_client || is_access_denied || is_custom) {
+ ++details_num;
+ errcode = box_error_code(error);
+ data_size += mp_sizeof_uint(MP_ERROR_DET_CODE);
+ data_size += mp_sizeof_uint(errcode);
+ if (is_custom) {
+ ++details_num;
+ data_size += mp_sizeof_uint(MP_ERROR_DET_CUSTOM_TYPE);
+ custom_type = box_error_custom_type(error);
+ data_size += mp_sizeof_str(strlen(custom_type));
+ } else if (is_access_denied) {
+ AccessDeniedError *ad_err = type_cast(AccessDeniedError,
+ error);
+ details_num += 3;
+ ad_obj_type = ad_err->object_type();
+ ad_obj_name = ad_err->object_name();
+ ad_access_type = ad_err->access_type();
+ data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_TYPE);
+ data_size += mp_sizeof_str(strlen(ad_obj_type));
+ data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_NAME);
+ data_size += mp_sizeof_str(strlen(ad_obj_name));
+ data_size += mp_sizeof_uint(MP_ERROR_DET_AD_ACCESS_TYPE);
+ data_size += mp_sizeof_str(strlen(ad_access_type));
+ }
+ }
+
+ data_size += mp_sizeof_map(details_num);
+ uint32_t data_size_ext = mp_sizeof_ext(data_size);
+ char *ptr = mpstream_reserve(stream, data_size_ext);
+
+ char *data = ptr;
+ data = mp_encode_extl(data, MP_ERROR, data_size);
+ data = mp_encode_map(data, details_num);
+ data = mp_encode_uint(data, MP_ERROR_DET_TYPE);
+ data = mp_encode_str(data, error->type->name,
+ strlen(error->type->name));
+ data = mp_encode_uint(data, MP_ERROR_DET_LINE);
+ data = mp_encode_uint(data, error->line);
+ data = mp_encode_uint(data, MP_ERROR_DET_FILE);
+ data = mp_encode_str(data, error->file, strlen(error->file));
+ data = mp_encode_uint(data, MP_ERROR_DET_REASON);
+ data = mp_encode_str(data, error->errmsg, strlen(error->errmsg));
+ data = mp_encode_uint(data, MP_ERROR_DET_ERRNO);
+ data = mp_encode_uint(data, error->saved_errno);
+
+ if (is_client || is_access_denied || is_custom) {
+ data = mp_encode_uint(data, MP_ERROR_DET_CODE);
+ data = mp_encode_uint(data, errcode);
+ if (is_custom) {
+ data = mp_encode_uint(data, MP_ERROR_DET_CUSTOM_TYPE);
+ data = mp_encode_str(data, custom_type,
+ strlen(custom_type));
+ } else if (is_access_denied) {
+ data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_TYPE);
+ data = mp_encode_str(data, ad_obj_type,
+ strlen(ad_obj_type));
+ data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_NAME);
+ data = mp_encode_str(data, ad_obj_name,
+ strlen(ad_obj_name));
+ data = mp_encode_uint(data, MP_ERROR_DET_AD_ACCESS_TYPE);
+ data = mp_encode_str(data, ad_access_type,
+ strlen(ad_access_type));
+ }
+ }
+
+ assert(data == ptr + data_size_ext);
+ mpstream_advance(stream, data_size_ext);
+}
+
+static struct error *
+build_error(struct mp_error *mp_error)
+{
+ /*
+ * For create an error the "raw" constructor using
+ * because OOM error must be throwed in OOM case.
+ * Bulders returns a pointer to the static OOM error
+ * in OOM case.
+ */
+ struct error *err = NULL;
+
+ if (strcmp(mp_error->error_type, "ClientError") == 0) {
+ ClientError *e = new ClientError(mp_error->file, mp_error->line,
+ ER_UNKNOWN);
+ e->m_errcode = mp_error->error_code;
+ err = e;
+ } else if (strcmp(mp_error->error_type, "CustomError") == 0) {
+ err = new CustomError(mp_error->file, mp_error->line,
+ mp_error->custom_type);
+ } else if (strcmp(mp_error->error_type, "AccessDeniedError") == 0) {
+ err = new AccessDeniedError(mp_error->file, mp_error->line,
+ mp_error->ad_access_type,
+ mp_error->ad_obj_type,
+ mp_error->ad_obj_name, "");
+ } else if (strcmp(mp_error->error_type, "XlogError") == 0) {
+ err = new XlogError(&type_XlogError, mp_error->file,
+ mp_error->line);
+ error_format_msg(err, "%s", mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "XlogGapError") == 0) {
+ err = new XlogGapError(mp_error->file, mp_error->line,
+ mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "SystemError") == 0) {
+ err = new SystemError(mp_error->file, mp_error->line,
+ "%s", mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "SocketError") == 0) {
+ err = new SocketError(mp_error->file, mp_error->line, "", "");
+ error_format_msg(err, "%s", mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "OutOfMemory") == 0) {
+ err = new OutOfMemory(mp_error->file, mp_error->line,
+ 0, "", "");
+ error_format_msg(err, "%s", mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "TimedOut") == 0) {
+ err = new TimedOut(mp_error->file, mp_error->line);
+ } else if (strcmp(mp_error->error_type, "ChannelIsClosed") == 0) {
+ err = new ChannelIsClosed(mp_error->file, mp_error->line);
+ } else if (strcmp(mp_error->error_type, "FiberIsCancelled") == 0) {
+ err = new FiberIsCancelled(mp_error->file, mp_error->line);
+ } else if (strcmp(mp_error->error_type, "LuajitError") == 0) {
+ err = new LuajitError(mp_error->file, mp_error->line,
+ mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "IllegalParams") == 0) {
+ err = new IllegalParams(mp_error->file, mp_error->line,
+ "%s", mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "CollationError") == 0) {
+ err = new CollationError(mp_error->file, mp_error->line,
+ "%s", mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "SwimError") == 0) {
+ err = new SwimError(mp_error->file, mp_error->line,
+ "%s", mp_error->reason);
+ } else if (strcmp(mp_error->error_type, "CryptoError") == 0) {
+ err = new CryptoError(mp_error->file, mp_error->line,
+ "%s", mp_error->reason);
+ }
+
+ if (err) {
+ err->saved_errno = mp_error->saved_errno;
+ error_format_msg(err, "%s", mp_error->reason);
+ }
+
+ return err;
+}
+
+struct error *
+error_unpack(const char **data, uint32_t len)
+{
+ const char *end = *data + len;
+ if (mp_typeof(**data) != MP_MAP) {
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_ERROR format");
+ return NULL;
+ }
+
+ struct mp_error mp_err;
+ mp_error_init(&mp_err);
+
+ uint32_t map_size = mp_decode_map(data);
+
+ struct error *err = NULL;
+ for (uint32_t i = 0; i < map_size; ++i) {
+ if (mp_typeof(**data) != MP_UINT) {
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_ERROR MsgPack format");
+ return NULL;
+ }
+
+ uint8_t key = mp_decode_uint(data);
+ const char *str;
+ uint32_t str_len;
+ switch(key) {
+ case MP_ERROR_DET_TYPE:
+ if (mp_typeof(**data) != MP_STR)
+ goto error;
+ str = mp_decode_str(data, &str_len);
+ mp_err.error_type = strndup(str, str_len);
+ break;
+ case MP_ERROR_DET_FILE:
+ if (mp_typeof(**data) != MP_STR)
+ goto error;
+ str = mp_decode_str(data, &str_len);
+ mp_err.file = strndup(str, str_len);
+ break;
+ case MP_ERROR_DET_LINE:
+ if (mp_typeof(**data) != MP_UINT)
+ goto error;
+ mp_err.line = mp_decode_uint(data);
+ break;
+ case MP_ERROR_DET_REASON:
+ if (mp_typeof(**data) != MP_STR)
+ goto error;
+ str = mp_decode_str(data, &str_len);
+ mp_err.reason = strndup(str, str_len);
+ break;
+ case MP_ERROR_DET_ERRNO:
+ if (mp_typeof(**data) != MP_UINT)
+ goto error;
+ mp_err.saved_errno = mp_decode_uint(data);
+ break;
+ case MP_ERROR_DET_CODE:
+ if (mp_typeof(**data) != MP_UINT)
+ goto error;
+ mp_err.error_code = mp_decode_uint(data);
+ break;
+ case MP_ERROR_DET_CUSTOM_TYPE:
+ if (mp_typeof(**data) != MP_STR)
+ goto error;
+ str = mp_decode_str(data, &str_len);
+ mp_err.custom_type = strndup(str, str_len);
+ break;
+ case MP_ERROR_DET_AD_OBJ_TYPE:
+ if (mp_typeof(**data) != MP_STR)
+ goto error;
+ str = mp_decode_str(data, &str_len);
+ mp_err.ad_obj_type = strndup(str, str_len);
+ break;
+ case MP_ERROR_DET_AD_OBJ_NAME:
+ if (mp_typeof(**data) != MP_STR)
+ goto error;
+ str = mp_decode_str(data, &str_len);
+ mp_err.ad_obj_name = strndup(str, str_len);
+ break;
+ case MP_ERROR_DET_AD_ACCESS_TYPE:
+ if (mp_typeof(**data) != MP_STR)
+ goto error;
+ str = mp_decode_str(data, &str_len);
+ mp_err.ad_access_type = strndup(str, str_len);
+ break;
+ default:
+ mp_next(data);
+ }
+ }
+
+ (void)end;
+ assert(*data == end);
+
+ err = build_error(&mp_err);
+ mp_error_cleanup(&mp_err);
+ return err;
+
+error:
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_ERROR MsgPack format");
+ return NULL;
+}
diff --git a/src/box/lua/mp_error.h b/src/box/lua/mp_error.h
new file mode 100644
index 0000000..2aa5cb9
--- /dev/null
+++ b/src/box/lua/mp_error.h
@@ -0,0 +1,49 @@
+#pragma once
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+#include <stdint.h>
+
+struct mpstream;
+
+void
+error_to_mpstream(struct error *error, struct mpstream *stream);
+
+struct error *
+error_unpack(const char **data, uint32_t len);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index aba906d..03b4b8a 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -290,20 +290,6 @@ tuple_to_mpstream(struct tuple *tuple, struct mpstream *stream)
mpstream_advance(stream, bsize);
}
-/* A MsgPack extensions handler that supports tuples */
-static enum mp_type
-luamp_encode_extension_box(struct lua_State *L, int idx,
- struct mpstream *stream)
-{
- struct tuple *tuple = luaT_istuple(L, idx);
- if (tuple != NULL) {
- tuple_to_mpstream(tuple, stream);
- return MP_ARRAY;
- }
-
- return MP_EXT;
-}
-
/**
* Convert a tuple into lua table. Named fields are stored as
* {name = value} pairs. Not named fields are stored as
@@ -582,8 +568,6 @@ box_lua_tuple_init(struct lua_State *L)
luaL_register_module(L, tuplelib_name, lbox_tuplelib);
lua_pop(L, 1);
- luamp_set_encode_extension(luamp_encode_extension_box);
-
tuple_serializer_update_options();
trigger_create(&tuple_serializer.update_trigger,
on_msgpack_serializer_update, NULL, NULL);
diff --git a/src/lib/core/mp_extension_types.h b/src/lib/core/mp_extension_types.h
index 7d42f21..e3ff9f5 100644
--- a/src/lib/core/mp_extension_types.h
+++ b/src/lib/core/mp_extension_types.h
@@ -43,6 +43,7 @@ enum mp_extension_type {
MP_UNKNOWN_EXTENSION = 0,
MP_DECIMAL = 1,
MP_UUID = 2,
+ MP_ERROR = 3,
mp_extension_type_MAX,
};
diff --git a/src/lua/error.c b/src/lua/error.c
index 18a990a..4a9ffc8 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -34,8 +34,6 @@
#include <fiber.h>
#include "utils.h"
-static int CTID_CONST_STRUCT_ERROR_REF = 0;
-
struct error *
luaL_iserror(struct lua_State *L, int narg)
{
diff --git a/src/lua/error.h b/src/lua/error.h
index 16cdaf7..4e4dc04 100644
--- a/src/lua/error.h
+++ b/src/lua/error.h
@@ -37,7 +37,6 @@
extern "C" {
#endif /* defined(__cplusplus) */
-
/** \cond public */
struct error;
@@ -62,6 +61,8 @@ void
luaT_pusherror(struct lua_State *L, struct error *e);
/** \endcond public */
+extern uint32_t CTID_CONST_STRUCT_ERROR_REF;
+
struct error *
luaL_iserror(struct lua_State *L, int narg);
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index b14361f..91560ef 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -195,6 +195,8 @@ restart: /* used by MP_EXT of unidentified subtype */
case MP_UUID:
mpstream_encode_uuid(stream, field->uuidval);
break;
+ case MP_ERROR:
+ return luamp_encode_extension(L, top, stream);
default:
/* Run trigger if type can't be encoded */
type = luamp_encode_extension(L, top, stream);
@@ -237,6 +239,7 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
return top_type;
}
+
void
luamp_decode(struct lua_State *L, struct luaL_serializer *cfg,
const char **data)
diff --git a/src/lua/utils.c b/src/lua/utils.c
index bd320f3..aaa8a3f 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -47,6 +47,7 @@ static uint32_t CTID_CHAR_PTR;
static uint32_t CTID_CONST_CHAR_PTR;
static uint32_t CTID_UUID;
uint32_t CTID_DECIMAL;
+uint32_t CTID_CONST_STRUCT_ERROR_REF;
void *
@@ -650,8 +651,6 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
struct serializer_opts *opts, int index,
struct luaL_field *field)
{
- /* opts will be used for encode MP_ERROR in the future */
- (void)opts;
if (index < 0)
index = lua_gettop(L) + index + 1;
@@ -759,6 +758,9 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
} else if (cd->ctypeid == CTID_UUID) {
field->ext_type = MP_UUID;
field->uuidval = (struct tt_uuid *) cdata;
+ } else if (cd->ctypeid == CTID_CONST_STRUCT_ERROR_REF &&
+ opts && opts->error_marshaling_enabled) {
+ field->ext_type = MP_ERROR;
} else {
field->ext_type = MP_UNKNOWN_EXTENSION;
}
diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
new file mode 100755
index 0000000..eb86bdc
--- /dev/null
+++ b/test/box-tap/extended_error.test.lua
@@ -0,0 +1,157 @@
+#! /usr/bin/env tarantool
+
+local netbox = require('net.box')
+local os = require('os')
+local tap = require('tap')
+local uri = require('uri')
+
+local test = tap.test('check an extended error')
+test:plan(4)
+
+
+function error_func()
+ box.error(box.error.PROC_LUA, "An old good error")
+end
+
+function custom_error_func()
+ box.error("My Custom Type", "A modern custom error")
+end
+
+function custom_error_func_2()
+ local err = box.error.new("My Custom Type", "A modern custom error")
+ return err
+end
+
+local function version_at_least(peer_version_str, major, minor, patch)
+ local major_p, minor_p, patch_p = string.match(peer_version_str,
+ "(%d)%.(%d)%.(%d)")
+ major_p = tonumber(major_p)
+ minor_p = tonumber(minor_p)
+ patch_p = tonumber(patch_p)
+
+ if major_p < major
+ or (major_p == major and minor_p < minor)
+ or (major_p == major and minor_p == minor and patch_p < patch) then
+ return false
+ end
+
+ return true
+end
+
+local function grant_func(user, name)
+ box.schema.func.create(name, {if_not_exists = true})
+ box.schema.user.grant(user, 'execute', 'function', name, {
+ if_not_exists = true
+ })
+end
+
+local function check_error(err, check_list)
+ if type(check_list) ~= 'table' then
+ return false
+ end
+
+ for k, v in pairs(check_list) do
+ if k == 'type' then
+ if err.base_type ~= v then
+ return false
+ end
+ elseif k == 'custom_type' then
+ if err.type ~= v then
+ return false
+ end
+ elseif k == 'message' then
+ if err.message ~= v then
+ return false
+ end
+ elseif k == 'trace' then
+ local trace = "File " .. err.trace[1]['file']
+ .. "\nLine " .. err.trace[1]['line']
+ if not string.match(trace, v) then
+ return false
+ end
+ elseif k == 'errno' then
+ if err.errno ~= v then
+ return false
+ end
+ elseif k == 'is_custom' then
+ if (err.base_type == 'CustomError') ~= v then
+ return false
+ end
+ else
+ return false
+ end
+ end
+
+ return true
+end
+
+local function test_old_transmission(host, port)
+ grant_func('guest', 'error_func')
+ grant_func('guest', 'custom_error_func_2')
+
+ local connection = netbox.connect(host, port)
+ local _, err = pcall(connection.call, connection, 'error_func')
+ local err_2 = connection:call('custom_error_func_2')
+
+
+ local check_list = {
+ type = 'ClientError',
+ message = 'An old good error',
+ trace = '^File builtin/box/net_box.lua\nLine %d+$',
+ is_custom = false
+ }
+
+ local check_result = check_error(err, check_list)
+ local check_result_2 = type(err_2) == 'string' and
+ err_2 == 'A modern custom error'
+
+ test:ok(check_result, 'Check the old transmission type(IPROTO_ERROR)')
+ test:ok(check_result_2, 'Check the old transmission type(IPROTO_OK)')
+
+ connection:close()
+end
+
+local function test_extended_transmission(host, port)
+ grant_func('guest', 'custom_error_func')
+ grant_func('guest', 'custom_error_func_2')
+ box.schema.user.grant('guest','read,write', 'space', '_session_settings')
+
+ local connection = netbox.connect(host, port)
+ connection.space._session_settings:update('error_marshaling_enabled',
+ {{'=', 2, true}})
+ local _, err = pcall(connection.call, connection, 'custom_error_func')
+ local err_2 = connection:call('custom_error_func_2')
+
+ local check_list = {
+ type = 'CustomError',
+ custom_type = 'My Custom Type',
+ message = 'A modern custom error',
+ trace = '^File builtin/box/net_box.lua\nLine %d+$',
+ is_custom = true
+ }
+
+ local check_list_2 = {
+ type = 'CustomError',
+ custom_type = 'My Custom Type',
+ message = 'A modern custom error',
+ trace = '.*extended_error.test.lua\nLine 21$',
+ is_custom = true
+ }
+ local check_result = check_error(err, check_list)
+ local check_result_2 = check_error(err_2, check_list_2)
+ test:ok(check_result, 'Check the extended transmission type(IPROTO_ERROR)')
+ test:ok(check_result_2, 'Check the extended transmission type(IPROTO_OK)')
+
+ connection:close()
+end
+
+box.cfg{
+ listen = os.getenv('LISTEN')
+}
+local host= uri.parse(box.cfg.listen).host or 'localhost'
+local port = uri.parse(box.cfg.listen).service
+
+test_extended_transmission(host, port)
+test_old_transmission(host, port)
+
+os.exit(test:check() and 0 or 1)
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding Leonid Vasiliev
@ 2020-04-17 0:58 ` Vladislav Shpilevoy
2020-04-18 17:46 ` lvasiliev
0 siblings, 1 reply; 16+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-17 0:58 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches
Thanks for the patch!
One major note: mpstream_iproto_encode_error() is not
updated at all. So the marshaling is still not complete.
Generally the patch is relatively fine. Still some
comments exist, but almost no major ones, except the
one above.
On 16/04/2020 19:38, Leonid Vasiliev wrote:
> Closes #4398
>
> @TarantoolBot document
> Title: msgpack format for MP_ERROR
>
> For sent an error over IProto MP_ERROR(0x03) subtype
> has been added to MsgPack MP_EXT.
> Now, if session setting 'error_marshaling_enabled'
> is true error will be encoded as:
> ```
> +--------+----------+========+
> | MP_EXT | MP_ERROR | MP_MAP |
> +--------+----------+========+
> ```
> At the map passes all necessary parameters (depending
> from the error type) to create a copy of the error
> on the client side.
> The necessary fields:
> - Error type
> - Trace(file name and line)
Since we decided to drop traceback for now, we should omit
this too, until it is designed properly. Better not to add them
now and add later with a thorough design, than add in a hurry
now, and then not be able to change it.
> - Error message
> - Errno
You need to document the exact numbers and types of every
of these fields.
> Additional fields:
> - ClientError code
> - Traceback (now only Lua traceback)
Traceback is not here anymore.
> - Custom type
> - Access Denied object type
> - Access Denied object name
> - Access Denied access type
Your error structure is flat, and is based entirely on number
keys. Probably a better option would be to store all type-specific
optional attributes with MP_STR keys and in a separate map.
Separate map would make it clear what options are type-specific,
and would simplify decoding on the client a bit.
Talking of numbers, there is the problem of introduction of new
attributes and of compatibility again. About compatibility all
my arguments are the same as for error types. If you will use
string keys for type-specific and potentially extendible fields,
it will simplify support in connectors. About problem of new
attributes introduction see example below:
Assume, we have AccessDeniedError, XlogGapError, and a SwimError.
Now assume they have some attributes encoded with these keys:
MP_ERROR_ATTR_ACCESS_DENIED_OBJ_TYPE = 0,
MP_ERROR_ATTR_ACCESS_DENIED_OBJ_NAME = 1,
MP_ERROR_ATTR_ACCESS_DENIED_ACC_TYPE = 2,
MP_ERROR_ATTR_XLOG_GAP_VCLOCK_START = 3,
MP_ERROR_ATTR_XLOG_GAP_VCLOCK_END = 4,
MP_ERROR_ATTR_SWIM_NODE_DEAD_UUID = 5,
And now we want to add a new attribute to the AccessDeniedError
object. We will need to make it having key 6. That separates "access
denied" attributes from each other. After a few more mutations
this will be a mess.
My proposal is this:
MP_ERROR: {
MP_ERROR_TYPE: ...,
MP_ERROR_CUSTOM_TYPE: ...,
MP_ERROR_TRACE: ...,
MP_ERROR_MESSAGE: ...,
MP_ERRNO: ...,
MP_ERROR_CODE: ...,
MP_ERROR_FIELDS: {
<MP_STR>: ...,
<MP_STR>: ...,
...
}
}
As you can see, I added MP_ERROR_FIELDS, and all type-specific
fields are present here as string key + some arbitrary values.
Easy to add new type-specific attributes.
But this is arguable. I rely on my recent experience with working
with Google Cloud where they return all information as 'string key' +
'string value' except HTTP codes, and it appeared to be easy to
handle. I advice you get a second opinion from Alexander T. and
probably from Kostja Nazarov. Maybe from Mons, he knows how to do
right lots of things.
Keep in mind this is not perf critical part, so we need to be on
the best compatibility side here.
See 5 comments below, and a commit on top of the branch. Looks like
I broke something in my commit so that the new test does not pass.
Could you please find a reason?
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index 63e8b82..de09389 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -386,6 +391,54 @@ static const struct luaL_Reg boxlib_backup[] = {
> {NULL, NULL}
> };
>
> +/**
> + * A MsgPack extensions handler that supports tuples and errors encode.
> + */
> +static enum mp_type
> +luamp_encode_extension_box(struct lua_State *L, int idx,
> + struct mpstream *stream)
1. Could you please move this function to a separate commit?
I see you do some tuple-related things here. Would be better to
make it beforehand to simplify this commit.
> +{
> + struct tuple *tuple = luaT_istuple(L, idx);
> + if (tuple != NULL) {
> + tuple_to_mpstream(tuple, stream);
> + return MP_ARRAY;
> + }
> +
> + if (current_session()->meta.serializer_opts.error_marshaling_enabled) {
> + struct error *err = luaL_iserror(L, idx);
> + if (err != NULL)
> + error_to_mpstream(err, stream);
> + }
> +
> + return MP_EXT;
> +}
> diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
> new file mode 100644
> index 0000000..2e15206
> --- /dev/null
> +++ b/src/box/lua/mp_error.cc
> @@ -0,0 +1,373 @@
> +static struct error *
> +build_error(struct mp_error *mp_error)
> +{
> + /*
> + * For create an error the "raw" constructor using
> + * because OOM error must be throwed in OOM case.
> + * Bulders returns a pointer to the static OOM error
> + * in OOM case.
2. Why do you want an exception out? That function is called from
error_unpack(), which is called from C. You can't throw here. The
function should be wrapped into a try-catch block. Am I missing
something?
In case there was OOM, should we return the OOM, or return NULL?
> + */
> + struct error *err = NULL;
> +
> + if (strcmp(mp_error->error_type, "ClientError") == 0) {
> + ClientError *e = new ClientError(mp_error->file, mp_error->line,
> + ER_UNKNOWN);
> + e->m_errcode = mp_error->error_code;
> + err = e;
> + } else if (strcmp(mp_error->error_type, "CustomError") == 0) {
> + err = new CustomError(mp_error->file, mp_error->line,
> + mp_error->custom_type);
> + } else if (strcmp(mp_error->error_type, "AccessDeniedError") == 0) {
> + err = new AccessDeniedError(mp_error->file, mp_error->line,
> + mp_error->ad_access_type,
> + mp_error->ad_obj_type,
> + mp_error->ad_obj_name, "");
> + } else if (strcmp(mp_error->error_type, "XlogError") == 0) {
> + err = new XlogError(&type_XlogError, mp_error->file,
> + mp_error->line);
> + error_format_msg(err, "%s", mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "XlogGapError") == 0) {
> + err = new XlogGapError(mp_error->file, mp_error->line,
> + mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "SystemError") == 0) {
> + err = new SystemError(mp_error->file, mp_error->line,
> + "%s", mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "SocketError") == 0) {
> + err = new SocketError(mp_error->file, mp_error->line, "", "");
> + error_format_msg(err, "%s", mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "OutOfMemory") == 0) {
> + err = new OutOfMemory(mp_error->file, mp_error->line,
> + 0, "", "");
> + error_format_msg(err, "%s", mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "TimedOut") == 0) {
> + err = new TimedOut(mp_error->file, mp_error->line);
> + } else if (strcmp(mp_error->error_type, "ChannelIsClosed") == 0) {
> + err = new ChannelIsClosed(mp_error->file, mp_error->line);
> + } else if (strcmp(mp_error->error_type, "FiberIsCancelled") == 0) {
> + err = new FiberIsCancelled(mp_error->file, mp_error->line);
> + } else if (strcmp(mp_error->error_type, "LuajitError") == 0) {
> + err = new LuajitError(mp_error->file, mp_error->line,
> + mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "IllegalParams") == 0) {
> + err = new IllegalParams(mp_error->file, mp_error->line,
> + "%s", mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "CollationError") == 0) {
> + err = new CollationError(mp_error->file, mp_error->line,
> + "%s", mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "SwimError") == 0) {
> + err = new SwimError(mp_error->file, mp_error->line,
> + "%s", mp_error->reason);
> + } else if (strcmp(mp_error->error_type, "CryptoError") == 0) {
> + err = new CryptoError(mp_error->file, mp_error->line,
> + "%s", mp_error->reason);
> + }
> +
> + if (err) {
> + err->saved_errno = mp_error->saved_errno;
> + error_format_msg(err, "%s", mp_error->reason);
> + }
> +
> + return err;
> +}
> +
> +struct error *
> +error_unpack(const char **data, uint32_t len)
> +{
> + const char *end = *data + len;
> + if (mp_typeof(**data) != MP_MAP) {
> + diag_set(ClientError, ER_INVALID_MSGPACK,
> + "Invalid MP_ERROR format");
> + return NULL;
> + }
> +
> + struct mp_error mp_err;
> + mp_error_init(&mp_err);
> +
> + uint32_t map_size = mp_decode_map(data);
> +
> + struct error *err = NULL;
> + for (uint32_t i = 0; i < map_size; ++i) {
> + if (mp_typeof(**data) != MP_UINT) {
> + diag_set(ClientError, ER_INVALID_MSGPACK,
> + "Invalid MP_ERROR MsgPack format");
> + return NULL;
> + }
> +
> + uint8_t key = mp_decode_uint(data);
> + const char *str;
> + uint32_t str_len;
> + switch(key) {
> + case MP_ERROR_DET_TYPE:
> + if (mp_typeof(**data) != MP_STR)
> + goto error;
3. That is actually an open question whether we should validate
MessagePack here. Currently none of MP_EXT decoders do that in
luamp_decode. Normal types also skip validation.
I am not saying it should not be fixed, but probably everywhere
at once, in a centralized way.
> + str = mp_decode_str(data, &str_len);
> + mp_err.error_type = strndup(str, str_len);
> + break;
> + case MP_ERROR_DET_FILE:
> + if (mp_typeof(**data) != MP_STR)
> + goto error;
> + str = mp_decode_str(data, &str_len);
> + mp_err.file = strndup(str, str_len);
> + break;
> diff --git a/src/lua/error.h b/src/lua/error.h
> index 16cdaf7..4e4dc04 100644
> --- a/src/lua/error.h
> +++ b/src/lua/error.h
> @@ -37,7 +37,6 @@
> extern "C" {
> #endif /* defined(__cplusplus) */
>
> -
4. Please, omit unnecessary diff.
> /** \cond public */
> struct error;
>
> diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
> new file mode 100755
> index 0000000..eb86bdc
> --- /dev/null
> +++ b/test/box-tap/extended_error.test.lua
5. Not sure how does it work now, taking into account that you
didn't update iproto part.
Please, add tests which use msgpack and msgpackffi. Without
network. Marshaling should affect it too.
Also add tests when happens when marshaling is disabled, and
yet a session tries to decode MP_ERROR. That may happen, if
it obtains the MessagePack generated in another session, or
if you turn marshaling on and off for one session.
====================
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index de09389ef..5e04987fe 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -423,7 +423,7 @@ luamp_decode_extension_box(struct lua_State *L, const char **data)
int8_t ext_type;
uint32_t len = mp_decode_extl(data, &ext_type);
- if(ext_type != MP_ERROR) {
+ if (ext_type != MP_ERROR) {
luaL_error(L, "Unsuported MsgPack extension type: %u",
ext_type);
return;
diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
index 2e1520621..38a6f6e98 100644
--- a/src/box/lua/mp_error.cc
+++ b/src/box/lua/mp_error.cc
@@ -33,21 +33,22 @@
#include "mpstream/mpstream.h"
#include "msgpuck.h"
#include "mp_extension_types.h"
+#include "fiber.h"
/**
* Keys used for encode/decode MP_ERROR.
*/
-enum mp_error_details {
- MP_ERROR_DET_TYPE,
- MP_ERROR_DET_FILE,
- MP_ERROR_DET_LINE,
- MP_ERROR_DET_REASON,
- MP_ERROR_DET_ERRNO,
- MP_ERROR_DET_CODE,
- MP_ERROR_DET_CUSTOM_TYPE,
- MP_ERROR_DET_AD_OBJ_TYPE,
- MP_ERROR_DET_AD_OBJ_NAME,
- MP_ERROR_DET_AD_ACCESS_TYPE
+enum {
+ MP_ERROR_TYPE,
+ MP_ERROR_FILE,
+ MP_ERROR_LINE,
+ MP_ERROR_REASON,
+ MP_ERROR_ERRNO,
+ MP_ERROR_CODE,
+ MP_ERROR_CUSTOM_TYPE,
+ MP_ERROR_AD_OBJ_TYPE,
+ MP_ERROR_AD_OBJ_NAME,
+ MP_ERROR_AD_ACCESS_TYPE
};
/**
@@ -58,40 +59,19 @@ struct mp_error {
uint32_t error_code;
uint32_t line;
uint32_t saved_errno;
- char *error_type;
- char *file;
- char *reason;
- char *custom_type;
- char *ad_obj_type;
- char *ad_obj_name;
- char *ad_access_type;
+ const char *error_type;
+ const char *file;
+ const char *reason;
+ const char *custom_type;
+ const char *ad_obj_type;
+ const char *ad_obj_name;
+ const char *ad_access_type;
};
static void
-mp_error_init(struct mp_error *mp_error)
+mp_error_create(struct mp_error *mp_error)
{
- mp_error->error_code = 0;
- mp_error->line = 0;
- mp_error->saved_errno = 0;
- mp_error->error_type = NULL;
- mp_error->file = NULL;
- mp_error->reason = NULL;
- mp_error->custom_type = NULL;
- mp_error->ad_obj_type = NULL;
- mp_error->ad_obj_name = NULL;
- mp_error->ad_access_type = NULL;
-}
-
-static void
-mp_error_cleanup(struct mp_error *mp_error)
-{
- free(mp_error->error_type);
- free(mp_error->file);
- free(mp_error->reason);
- free(mp_error->custom_type);
- free(mp_error->ad_obj_type);
- free(mp_error->ad_obj_name);
- free(mp_error->ad_access_type);
+ memset(mp_error, 0, sizeof(*mp_error));
}
void
@@ -118,25 +98,25 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
uint32_t details_num = 5;
uint32_t data_size = 0;
- data_size += mp_sizeof_uint(MP_ERROR_DET_TYPE);
+ data_size += mp_sizeof_uint(MP_ERROR_TYPE);
data_size += mp_sizeof_str(strlen(error->type->name));
- data_size += mp_sizeof_uint(MP_ERROR_DET_LINE);
+ data_size += mp_sizeof_uint(MP_ERROR_LINE);
data_size += mp_sizeof_uint(error->line);
- data_size += mp_sizeof_uint(MP_ERROR_DET_FILE);
+ data_size += mp_sizeof_uint(MP_ERROR_FILE);
data_size += mp_sizeof_str(strlen(error->file));
- data_size += mp_sizeof_uint(MP_ERROR_DET_REASON);
+ data_size += mp_sizeof_uint(MP_ERROR_REASON);
data_size += mp_sizeof_str(strlen(error->errmsg));
- data_size += mp_sizeof_uint(MP_ERROR_DET_ERRNO);
+ data_size += mp_sizeof_uint(MP_ERROR_ERRNO);
data_size += mp_sizeof_uint(error->saved_errno);
if (is_client || is_access_denied || is_custom) {
++details_num;
errcode = box_error_code(error);
- data_size += mp_sizeof_uint(MP_ERROR_DET_CODE);
+ data_size += mp_sizeof_uint(MP_ERROR_CODE);
data_size += mp_sizeof_uint(errcode);
if (is_custom) {
++details_num;
- data_size += mp_sizeof_uint(MP_ERROR_DET_CUSTOM_TYPE);
+ data_size += mp_sizeof_uint(MP_ERROR_CUSTOM_TYPE);
custom_type = box_error_custom_type(error);
data_size += mp_sizeof_str(strlen(custom_type));
} else if (is_access_denied) {
@@ -146,11 +126,11 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
ad_obj_type = ad_err->object_type();
ad_obj_name = ad_err->object_name();
ad_access_type = ad_err->access_type();
- data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_TYPE);
+ data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_TYPE);
data_size += mp_sizeof_str(strlen(ad_obj_type));
- data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_NAME);
+ data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_NAME);
data_size += mp_sizeof_str(strlen(ad_obj_name));
- data_size += mp_sizeof_uint(MP_ERROR_DET_AD_ACCESS_TYPE);
+ data_size += mp_sizeof_uint(MP_ERROR_AD_ACCESS_TYPE);
data_size += mp_sizeof_str(strlen(ad_access_type));
}
}
@@ -162,33 +142,33 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
char *data = ptr;
data = mp_encode_extl(data, MP_ERROR, data_size);
data = mp_encode_map(data, details_num);
- data = mp_encode_uint(data, MP_ERROR_DET_TYPE);
+ data = mp_encode_uint(data, MP_ERROR_TYPE);
data = mp_encode_str(data, error->type->name,
strlen(error->type->name));
- data = mp_encode_uint(data, MP_ERROR_DET_LINE);
+ data = mp_encode_uint(data, MP_ERROR_LINE);
data = mp_encode_uint(data, error->line);
- data = mp_encode_uint(data, MP_ERROR_DET_FILE);
+ data = mp_encode_uint(data, MP_ERROR_FILE);
data = mp_encode_str(data, error->file, strlen(error->file));
- data = mp_encode_uint(data, MP_ERROR_DET_REASON);
+ data = mp_encode_uint(data, MP_ERROR_REASON);
data = mp_encode_str(data, error->errmsg, strlen(error->errmsg));
- data = mp_encode_uint(data, MP_ERROR_DET_ERRNO);
+ data = mp_encode_uint(data, MP_ERROR_ERRNO);
data = mp_encode_uint(data, error->saved_errno);
if (is_client || is_access_denied || is_custom) {
- data = mp_encode_uint(data, MP_ERROR_DET_CODE);
+ data = mp_encode_uint(data, MP_ERROR_CODE);
data = mp_encode_uint(data, errcode);
if (is_custom) {
- data = mp_encode_uint(data, MP_ERROR_DET_CUSTOM_TYPE);
+ data = mp_encode_uint(data, MP_ERROR_CUSTOM_TYPE);
data = mp_encode_str(data, custom_type,
strlen(custom_type));
} else if (is_access_denied) {
- data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_TYPE);
+ data = mp_encode_uint(data, MP_ERROR_AD_OBJ_TYPE);
data = mp_encode_str(data, ad_obj_type,
strlen(ad_obj_type));
- data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_NAME);
+ data = mp_encode_uint(data, MP_ERROR_AD_OBJ_NAME);
data = mp_encode_str(data, ad_obj_name,
strlen(ad_obj_name));
- data = mp_encode_uint(data, MP_ERROR_DET_AD_ACCESS_TYPE);
+ data = mp_encode_uint(data, MP_ERROR_AD_ACCESS_TYPE);
data = mp_encode_str(data, ad_access_type,
strlen(ad_access_type));
}
@@ -202,9 +182,9 @@ static struct error *
build_error(struct mp_error *mp_error)
{
/*
- * For create an error the "raw" constructor using
- * because OOM error must be throwed in OOM case.
- * Bulders returns a pointer to the static OOM error
+ * To create an error the "raw" constructor using
+ * because OOM error must be thrown in OOM case.
+ * Builders returns a pointer to the static OOM error
* in OOM case.
*/
struct error *err = NULL;
@@ -270,8 +250,21 @@ build_error(struct mp_error *mp_error)
return err;
}
+static inline const char *
+region_strdup(struct region *region, const char *str, uint32_t len)
+{
+ char *res = (char *)region_alloc(region, len + 1);
+ if (res == NULL) {
+ diag_set(OutOfMemory, len + 1, "region_alloc", "res");
+ return NULL;
+ }
+ memcpy(res, str, len);
+ res[len] = 0;
+ return res;
+}
+
struct error *
-error_unpack(const char **data, uint32_t len)
+error_unpack_xc(const char **data, uint32_t len)
{
const char *end = *data + len;
if (mp_typeof(**data) != MP_MAP) {
@@ -281,8 +274,9 @@ error_unpack(const char **data, uint32_t len)
}
struct mp_error mp_err;
- mp_error_init(&mp_err);
-
+ mp_error_create(&mp_err);
+ struct region *region = &fiber()->gc;
+ uint32_t region_svp = region_used(region);
uint32_t map_size = mp_decode_map(data);
struct error *err = NULL;
@@ -290,69 +284,87 @@ error_unpack(const char **data, uint32_t len)
if (mp_typeof(**data) != MP_UINT) {
diag_set(ClientError, ER_INVALID_MSGPACK,
"Invalid MP_ERROR MsgPack format");
- return NULL;
+ goto finish;
}
- uint8_t key = mp_decode_uint(data);
+ uint64_t key = mp_decode_uint(data);
const char *str;
uint32_t str_len;
switch(key) {
- case MP_ERROR_DET_TYPE:
+ case MP_ERROR_TYPE:
if (mp_typeof(**data) != MP_STR)
goto error;
str = mp_decode_str(data, &str_len);
- mp_err.error_type = strndup(str, str_len);
+ mp_err.error_type = region_strdup(region, str, str_len);
+ if (mp_err.error_type == NULL)
+ goto finish;
break;
- case MP_ERROR_DET_FILE:
+ case MP_ERROR_FILE:
if (mp_typeof(**data) != MP_STR)
goto error;
str = mp_decode_str(data, &str_len);
- mp_err.file = strndup(str, str_len);
+ mp_err.file = region_strdup(region, str, str_len);
+ if (mp_err.file == NULL)
+ goto finish;
break;
- case MP_ERROR_DET_LINE:
+ case MP_ERROR_LINE:
if (mp_typeof(**data) != MP_UINT)
goto error;
mp_err.line = mp_decode_uint(data);
break;
- case MP_ERROR_DET_REASON:
+ case MP_ERROR_REASON:
if (mp_typeof(**data) != MP_STR)
goto error;
str = mp_decode_str(data, &str_len);
- mp_err.reason = strndup(str, str_len);
+ mp_err.reason = region_strdup(region, str, str_len);
+ if (mp_err.reason == NULL)
+ goto finish;
break;
- case MP_ERROR_DET_ERRNO:
+ case MP_ERROR_ERRNO:
if (mp_typeof(**data) != MP_UINT)
goto error;
mp_err.saved_errno = mp_decode_uint(data);
break;
- case MP_ERROR_DET_CODE:
+ case MP_ERROR_CODE:
if (mp_typeof(**data) != MP_UINT)
goto error;
mp_err.error_code = mp_decode_uint(data);
break;
- case MP_ERROR_DET_CUSTOM_TYPE:
+ case MP_ERROR_CUSTOM_TYPE:
if (mp_typeof(**data) != MP_STR)
goto error;
str = mp_decode_str(data, &str_len);
- mp_err.custom_type = strndup(str, str_len);
+ mp_err.custom_type = region_strdup(region, str,
+ str_len);
+ if (mp_err.custom_type == NULL)
+ goto finish;
break;
- case MP_ERROR_DET_AD_OBJ_TYPE:
+ case MP_ERROR_AD_OBJ_TYPE:
if (mp_typeof(**data) != MP_STR)
goto error;
str = mp_decode_str(data, &str_len);
- mp_err.ad_obj_type = strndup(str, str_len);
+ mp_err.ad_obj_type = region_strdup(region, str,
+ str_len);
+ if (mp_err.ad_obj_type == NULL)
+ goto finish;
break;
- case MP_ERROR_DET_AD_OBJ_NAME:
+ case MP_ERROR_AD_OBJ_NAME:
if (mp_typeof(**data) != MP_STR)
goto error;
str = mp_decode_str(data, &str_len);
- mp_err.ad_obj_name = strndup(str, str_len);
+ mp_err.ad_obj_name = region_strdup(region, str,
+ str_len);
+ if (mp_err.ad_obj_name == NULL)
+ goto finish;
break;
- case MP_ERROR_DET_AD_ACCESS_TYPE:
+ case MP_ERROR_AD_ACCESS_TYPE:
if (mp_typeof(**data) != MP_STR)
goto error;
str = mp_decode_str(data, &str_len);
- mp_err.ad_access_type = strndup(str, str_len);
+ mp_err.ad_access_type = region_strdup(region, str,
+ str_len);
+ if (mp_err.ad_access_type == NULL)
+ goto finish;
break;
default:
mp_next(data);
@@ -363,11 +375,22 @@ error_unpack(const char **data, uint32_t len)
assert(*data == end);
err = build_error(&mp_err);
- mp_error_cleanup(&mp_err);
+finish:
+ region_truncate(region, region_svp);
return err;
error:
diag_set(ClientError, ER_INVALID_MSGPACK,
"Invalid MP_ERROR MsgPack format");
- return NULL;
+ goto finish;
+}
+
+struct error *
+error_unpack(const char **data, uint32_t len)
+{
+ try {
+ return error_unpack_xc(data, len);
+ } catch (OutOfMemory *e) {
+ return e;
+ }
}
diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
index eb86bdc76..8a4f79c04 100755
--- a/test/box-tap/extended_error.test.lua
+++ b/test/box-tap/extended_error.test.lua
@@ -3,12 +3,10 @@
local netbox = require('net.box')
local os = require('os')
local tap = require('tap')
-local uri = require('uri')
local test = tap.test('check an extended error')
test:plan(4)
-
function error_func()
box.error(box.error.PROC_LUA, "An old good error")
end
@@ -22,22 +20,6 @@ function custom_error_func_2()
return err
end
-local function version_at_least(peer_version_str, major, minor, patch)
- local major_p, minor_p, patch_p = string.match(peer_version_str,
- "(%d)%.(%d)%.(%d)")
- major_p = tonumber(major_p)
- minor_p = tonumber(minor_p)
- patch_p = tonumber(patch_p)
-
- if major_p < major
- or (major_p == major and minor_p < minor)
- or (major_p == major and minor_p == minor and patch_p < patch) then
- return false
- end
-
- return true
-end
-
local function grant_func(user, name)
box.schema.func.create(name, {if_not_exists = true})
box.schema.user.grant(user, 'execute', 'function', name, {
@@ -85,15 +67,14 @@ local function check_error(err, check_list)
return true
end
-local function test_old_transmission(host, port)
+local function test_old_transmission()
grant_func('guest', 'error_func')
grant_func('guest', 'custom_error_func_2')
- local connection = netbox.connect(host, port)
+ local connection = netbox.connect(box.cfg.listen)
local _, err = pcall(connection.call, connection, 'error_func')
local err_2 = connection:call('custom_error_func_2')
-
local check_list = {
type = 'ClientError',
message = 'An old good error',
@@ -111,12 +92,12 @@ local function test_old_transmission(host, port)
connection:close()
end
-local function test_extended_transmission(host, port)
+local function test_extended_transmission()
grant_func('guest', 'custom_error_func')
grant_func('guest', 'custom_error_func_2')
box.schema.user.grant('guest','read,write', 'space', '_session_settings')
- local connection = netbox.connect(host, port)
+ local connection = netbox.connect(box.cfg.listen)
connection.space._session_settings:update('error_marshaling_enabled',
{{'=', 2, true}})
local _, err = pcall(connection.call, connection, 'custom_error_func')
@@ -148,10 +129,8 @@ end
box.cfg{
listen = os.getenv('LISTEN')
}
-local host= uri.parse(box.cfg.listen).host or 'localhost'
-local port = uri.parse(box.cfg.listen).service
-test_extended_transmission(host, port)
-test_old_transmission(host, port)
+test_extended_transmission()
+test_old_transmission()
os.exit(test:check() and 0 or 1)
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding
2020-04-17 0:58 ` Vladislav Shpilevoy
@ 2020-04-18 17:46 ` lvasiliev
0 siblings, 0 replies; 16+ messages in thread
From: lvasiliev @ 2020-04-18 17:46 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
On 17.04.2020 3:58, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> One major note: mpstream_iproto_encode_error() is not
> updated at all. So the marshaling is still not complete.
>
I tried. If we add mp_ext to IPROTO_ERROR, we’ll break old clients.
> Generally the patch is relatively fine. Still some
> comments exist, but almost no major ones, except the
> one above.
>
> On 16/04/2020 19:38, Leonid Vasiliev wrote:
>> Closes #4398
>>
>> @TarantoolBot document
>> Title: msgpack format for MP_ERROR
>>
>> For sent an error over IProto MP_ERROR(0x03) subtype
>> has been added to MsgPack MP_EXT.
>> Now, if session setting 'error_marshaling_enabled'
>> is true error will be encoded as:
>> ```
>> +--------+----------+========+
>> | MP_EXT | MP_ERROR | MP_MAP |
>> +--------+----------+========+
>> ```
>> At the map passes all necessary parameters (depending
>> from the error type) to create a copy of the error
>> on the client side.
>> The necessary fields:
>> - Error type
>> - Trace(file name and line)
>
> Since we decided to drop traceback for now, we should omit
> this too, until it is designed properly. Better not to add them
> now and add later with a thorough design, than add in a hurry
> now, and then not be able to change it.
>
>> - Error message
>> - Errno
>
> You need to document the exact numbers and types of every
> of these fields.
>
Added
>> Additional fields:
>> - ClientError code
>> - Traceback (now only Lua traceback)
>
> Traceback is not here anymore.
>
Discussed
>> - Custom type
>> - Access Denied object type
>> - Access Denied object name
>> - Access Denied access type
>
> Your error structure is flat, and is based entirely on number
> keys. Probably a better option would be to store all type-specific
> optional attributes with MP_STR keys and in a separate map.
>
> Separate map would make it clear what options are type-specific,
> and would simplify decoding on the client a bit.
>
> Talking of numbers, there is the problem of introduction of new
> attributes and of compatibility again. About compatibility all
> my arguments are the same as for error types. If you will use
> string keys for type-specific and potentially extendible fields,
> it will simplify support in connectors. About problem of new
> attributes introduction see example below:
>
> Assume, we have AccessDeniedError, XlogGapError, and a SwimError.
> Now assume they have some attributes encoded with these keys:
>
> MP_ERROR_ATTR_ACCESS_DENIED_OBJ_TYPE = 0,
> MP_ERROR_ATTR_ACCESS_DENIED_OBJ_NAME = 1,
> MP_ERROR_ATTR_ACCESS_DENIED_ACC_TYPE = 2,
> MP_ERROR_ATTR_XLOG_GAP_VCLOCK_START = 3,
> MP_ERROR_ATTR_XLOG_GAP_VCLOCK_END = 4,
> MP_ERROR_ATTR_SWIM_NODE_DEAD_UUID = 5,
>
> And now we want to add a new attribute to the AccessDeniedError
> object. We will need to make it having key 6. That separates "access
> denied" attributes from each other. After a few more mutations
> this will be a mess.
>
> My proposal is this:
>
> MP_ERROR: {
> MP_ERROR_TYPE: ...,
> MP_ERROR_CUSTOM_TYPE: ...,
> MP_ERROR_TRACE: ...,
> MP_ERROR_MESSAGE: ...,
> MP_ERRNO: ...,
> MP_ERROR_CODE: ...,
> MP_ERROR_FIELDS: {
> <MP_STR>: ...,
> <MP_STR>: ...,
> ...
> }
> }
>
Implemented
> As you can see, I added MP_ERROR_FIELDS, and all type-specific
> fields are present here as string key + some arbitrary values.
> Easy to add new type-specific attributes.
>
> But this is arguable. I rely on my recent experience with working
> with Google Cloud where they return all information as 'string key' +
> 'string value' except HTTP codes, and it appeared to be easy to
> handle. I advice you get a second opinion from Alexander T. and
> probably from Kostja Nazarov. Maybe from Mons, he knows how to do
> right lots of things.
>
> Keep in mind this is not perf critical part, so we need to be on
> the best compatibility side here.
>
> See 5 comments below, and a commit on top of the branch. Looks like
> I broke something in my commit so that the new test does not pass.
> Could you please find a reason?
>
>> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
>> index 63e8b82..de09389 100644
>> --- a/src/box/lua/init.c
>> +++ b/src/box/lua/init.c
>> @@ -386,6 +391,54 @@ static const struct luaL_Reg boxlib_backup[] = {
>> {NULL, NULL}
>> };
>>
>> +/**
>> + * A MsgPack extensions handler that supports tuples and errors encode.
>> + */
>> +static enum mp_type
>> +luamp_encode_extension_box(struct lua_State *L, int idx,
>> + struct mpstream *stream)
>
> 1. Could you please move this function to a separate commit?
> I see you do some tuple-related things here. Would be better to
> make it beforehand to simplify this commit.
>
With a separate commit, we move one function from file A to file B for
modify it in the next commit. Sounds weird.
>> +{
>> + struct tuple *tuple = luaT_istuple(L, idx);
>> + if (tuple != NULL) {
>> + tuple_to_mpstream(tuple, stream);
>> + return MP_ARRAY;
>> + }
>> +
>> + if (current_session()->meta.serializer_opts.error_marshaling_enabled) {
>> + struct error *err = luaL_iserror(L, idx);
>> + if (err != NULL)
>> + error_to_mpstream(err, stream);
>> + }
>> +
>> + return MP_EXT;
>> +}
>> diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
>> new file mode 100644
>> index 0000000..2e15206
>> --- /dev/null
>> +++ b/src/box/lua/mp_error.cc
>> @@ -0,0 +1,373 @@
>> +static struct error *
>> +build_error(struct mp_error *mp_error)
>> +{
>> + /*
>> + * For create an error the "raw" constructor using
>> + * because OOM error must be throwed in OOM case.
>> + * Bulders returns a pointer to the static OOM error
>> + * in OOM case.
>
> 2. Why do you want an exception out? That function is called from
> error_unpack(), which is called from C. You can't throw here. The
> function should be wrapped into a try-catch block. Am I missing
> something?
>
> In case there was OOM, should we return the OOM, or return NULL?
Null and set OOM to diag.
>
>> + */
>> + struct error *err = NULL;
>> +
>> + if (strcmp(mp_error->error_type, "ClientError") == 0) {
>> + ClientError *e = new ClientError(mp_error->file, mp_error->line,
>> + ER_UNKNOWN);
>> + e->m_errcode = mp_error->error_code;
>> + err = e;
>> + } else if (strcmp(mp_error->error_type, "CustomError") == 0) {
>> + err = new CustomError(mp_error->file, mp_error->line,
>> + mp_error->custom_type);
>> + } else if (strcmp(mp_error->error_type, "AccessDeniedError") == 0) {
>> + err = new AccessDeniedError(mp_error->file, mp_error->line,
>> + mp_error->ad_access_type,
>> + mp_error->ad_obj_type,
>> + mp_error->ad_obj_name, "");
>> + } else if (strcmp(mp_error->error_type, "XlogError") == 0) {
>> + err = new XlogError(&type_XlogError, mp_error->file,
>> + mp_error->line);
>> + error_format_msg(err, "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "XlogGapError") == 0) {
>> + err = new XlogGapError(mp_error->file, mp_error->line,
>> + mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "SystemError") == 0) {
>> + err = new SystemError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "SocketError") == 0) {
>> + err = new SocketError(mp_error->file, mp_error->line, "", "");
>> + error_format_msg(err, "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "OutOfMemory") == 0) {
>> + err = new OutOfMemory(mp_error->file, mp_error->line,
>> + 0, "", "");
>> + error_format_msg(err, "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "TimedOut") == 0) {
>> + err = new TimedOut(mp_error->file, mp_error->line);
>> + } else if (strcmp(mp_error->error_type, "ChannelIsClosed") == 0) {
>> + err = new ChannelIsClosed(mp_error->file, mp_error->line);
>> + } else if (strcmp(mp_error->error_type, "FiberIsCancelled") == 0) {
>> + err = new FiberIsCancelled(mp_error->file, mp_error->line);
>> + } else if (strcmp(mp_error->error_type, "LuajitError") == 0) {
>> + err = new LuajitError(mp_error->file, mp_error->line,
>> + mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "IllegalParams") == 0) {
>> + err = new IllegalParams(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "CollationError") == 0) {
>> + err = new CollationError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "SwimError") == 0) {
>> + err = new SwimError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "CryptoError") == 0) {
>> + err = new CryptoError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + }
>> +
>> + if (err) {
>> + err->saved_errno = mp_error->saved_errno;
>> + error_format_msg(err, "%s", mp_error->reason);
>> + }
>> +
>> + return err;
>> +}
>> +
>> +struct error *
>> +error_unpack(const char **data, uint32_t len)
>> +{
>> + const char *end = *data + len;
>> + if (mp_typeof(**data) != MP_MAP) {
>> + diag_set(ClientError, ER_INVALID_MSGPACK,
>> + "Invalid MP_ERROR format");
>> + return NULL;
>> + }
>> +
>> + struct mp_error mp_err;
>> + mp_error_init(&mp_err);
>> +
>> + uint32_t map_size = mp_decode_map(data);
>> +
>> + struct error *err = NULL;
>> + for (uint32_t i = 0; i < map_size; ++i) {
>> + if (mp_typeof(**data) != MP_UINT) {
>> + diag_set(ClientError, ER_INVALID_MSGPACK,
>> + "Invalid MP_ERROR MsgPack format");
>> + return NULL;
>> + }
>> +
>> + uint8_t key = mp_decode_uint(data);
>> + const char *str;
>> + uint32_t str_len;
>> + switch(key) {
>> + case MP_ERROR_DET_TYPE:
>> + if (mp_typeof(**data) != MP_STR)
>> + goto error;
>
> 3. That is actually an open question whether we should validate
> MessagePack here. Currently none of MP_EXT decoders do that in
> luamp_decode. Normal types also skip validation.
>
> I am not saying it should not be fixed, but probably everywhere
> at once, in a centralized way.
>
>> + str = mp_decode_str(data, &str_len);
>> + mp_err.error_type = strndup(str, str_len);
>> + break;
>> + case MP_ERROR_DET_FILE:
>> + if (mp_typeof(**data) != MP_STR)
>> + goto error;
>> + str = mp_decode_str(data, &str_len);
>> + mp_err.file = strndup(str, str_len);
>> + break;
>> diff --git a/src/lua/error.h b/src/lua/error.h
>> index 16cdaf7..4e4dc04 100644
>> --- a/src/lua/error.h
>> +++ b/src/lua/error.h
>> @@ -37,7 +37,6 @@
>> extern "C" {
>> #endif /* defined(__cplusplus) */
>>
>> -
>
> 4. Please, omit unnecessary diff.
>
>> /** \cond public */
>> struct error;
>>
>> diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
>> new file mode 100755
>> index 0000000..eb86bdc
>> --- /dev/null
>> +++ b/test/box-tap/extended_error.test.lua
>
> 5. Not sure how does it work now, taking into account that you
> didn't update iproto part.
>
> Please, add tests which use msgpack and msgpackffi. Without
> network. Marshaling should affect it too.
>
> Also add tests when happens when marshaling is disabled, and
> yet a session tries to decode MP_ERROR. That may happen, if
> it obtains the MessagePack generated in another session, or
> if you turn marshaling on and off for one session.
Tests inprogress.
>
> ====================
>
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index de09389ef..5e04987fe 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -423,7 +423,7 @@ luamp_decode_extension_box(struct lua_State *L, const char **data)
> int8_t ext_type;
> uint32_t len = mp_decode_extl(data, &ext_type);
>
> - if(ext_type != MP_ERROR) {
> + if (ext_type != MP_ERROR) {
> luaL_error(L, "Unsuported MsgPack extension type: %u",
> ext_type);
> return;
> diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
> index 2e1520621..38a6f6e98 100644
> --- a/src/box/lua/mp_error.cc
> +++ b/src/box/lua/mp_error.cc
> @@ -33,21 +33,22 @@
> #include "mpstream/mpstream.h"
> #include "msgpuck.h"
> #include "mp_extension_types.h"
> +#include "fiber.h"
>
> /**
> * Keys used for encode/decode MP_ERROR.
> */
> -enum mp_error_details {
> - MP_ERROR_DET_TYPE,
> - MP_ERROR_DET_FILE,
> - MP_ERROR_DET_LINE,
> - MP_ERROR_DET_REASON,
> - MP_ERROR_DET_ERRNO,
> - MP_ERROR_DET_CODE,
> - MP_ERROR_DET_CUSTOM_TYPE,
> - MP_ERROR_DET_AD_OBJ_TYPE,
> - MP_ERROR_DET_AD_OBJ_NAME,
> - MP_ERROR_DET_AD_ACCESS_TYPE
> +enum {
> + MP_ERROR_TYPE,
> + MP_ERROR_FILE,
> + MP_ERROR_LINE,
> + MP_ERROR_REASON,
> + MP_ERROR_ERRNO,
> + MP_ERROR_CODE,
> + MP_ERROR_CUSTOM_TYPE,
> + MP_ERROR_AD_OBJ_TYPE,
> + MP_ERROR_AD_OBJ_NAME,
> + MP_ERROR_AD_ACCESS_TYPE
> };
>
> /**
> @@ -58,40 +59,19 @@ struct mp_error {
> uint32_t error_code;
> uint32_t line;
> uint32_t saved_errno;
> - char *error_type;
> - char *file;
> - char *reason;
> - char *custom_type;
> - char *ad_obj_type;
> - char *ad_obj_name;
> - char *ad_access_type;
> + const char *error_type;
> + const char *file;
> + const char *reason;
> + const char *custom_type;
> + const char *ad_obj_type;
> + const char *ad_obj_name;
> + const char *ad_access_type;
> };
>
> static void
> -mp_error_init(struct mp_error *mp_error)
> +mp_error_create(struct mp_error *mp_error)
> {
> - mp_error->error_code = 0;
> - mp_error->line = 0;
> - mp_error->saved_errno = 0;
> - mp_error->error_type = NULL;
> - mp_error->file = NULL;
> - mp_error->reason = NULL;
> - mp_error->custom_type = NULL;
> - mp_error->ad_obj_type = NULL;
> - mp_error->ad_obj_name = NULL;
> - mp_error->ad_access_type = NULL;
> -}
> -
> -static void
> -mp_error_cleanup(struct mp_error *mp_error)
> -{
> - free(mp_error->error_type);
> - free(mp_error->file);
> - free(mp_error->reason);
> - free(mp_error->custom_type);
> - free(mp_error->ad_obj_type);
> - free(mp_error->ad_obj_name);
> - free(mp_error->ad_access_type);
> + memset(mp_error, 0, sizeof(*mp_error));
> }
>
> void
> @@ -118,25 +98,25 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
> uint32_t details_num = 5;
> uint32_t data_size = 0;
>
> - data_size += mp_sizeof_uint(MP_ERROR_DET_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_TYPE);
> data_size += mp_sizeof_str(strlen(error->type->name));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_LINE);
> + data_size += mp_sizeof_uint(MP_ERROR_LINE);
> data_size += mp_sizeof_uint(error->line);
> - data_size += mp_sizeof_uint(MP_ERROR_DET_FILE);
> + data_size += mp_sizeof_uint(MP_ERROR_FILE);
> data_size += mp_sizeof_str(strlen(error->file));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_REASON);
> + data_size += mp_sizeof_uint(MP_ERROR_REASON);
> data_size += mp_sizeof_str(strlen(error->errmsg));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_ERRNO);
> + data_size += mp_sizeof_uint(MP_ERROR_ERRNO);
> data_size += mp_sizeof_uint(error->saved_errno);
>
> if (is_client || is_access_denied || is_custom) {
> ++details_num;
> errcode = box_error_code(error);
> - data_size += mp_sizeof_uint(MP_ERROR_DET_CODE);
> + data_size += mp_sizeof_uint(MP_ERROR_CODE);
> data_size += mp_sizeof_uint(errcode);
> if (is_custom) {
> ++details_num;
> - data_size += mp_sizeof_uint(MP_ERROR_DET_CUSTOM_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_CUSTOM_TYPE);
> custom_type = box_error_custom_type(error);
> data_size += mp_sizeof_str(strlen(custom_type));
> } else if (is_access_denied) {
> @@ -146,11 +126,11 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
> ad_obj_type = ad_err->object_type();
> ad_obj_name = ad_err->object_name();
> ad_access_type = ad_err->access_type();
> - data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_TYPE);
> data_size += mp_sizeof_str(strlen(ad_obj_type));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_NAME);
> + data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_NAME);
> data_size += mp_sizeof_str(strlen(ad_obj_name));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_AD_ACCESS_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_AD_ACCESS_TYPE);
> data_size += mp_sizeof_str(strlen(ad_access_type));
> }
> }
> @@ -162,33 +142,33 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
> char *data = ptr;
> data = mp_encode_extl(data, MP_ERROR, data_size);
> data = mp_encode_map(data, details_num);
> - data = mp_encode_uint(data, MP_ERROR_DET_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_TYPE);
> data = mp_encode_str(data, error->type->name,
> strlen(error->type->name));
> - data = mp_encode_uint(data, MP_ERROR_DET_LINE);
> + data = mp_encode_uint(data, MP_ERROR_LINE);
> data = mp_encode_uint(data, error->line);
> - data = mp_encode_uint(data, MP_ERROR_DET_FILE);
> + data = mp_encode_uint(data, MP_ERROR_FILE);
> data = mp_encode_str(data, error->file, strlen(error->file));
> - data = mp_encode_uint(data, MP_ERROR_DET_REASON);
> + data = mp_encode_uint(data, MP_ERROR_REASON);
> data = mp_encode_str(data, error->errmsg, strlen(error->errmsg));
> - data = mp_encode_uint(data, MP_ERROR_DET_ERRNO);
> + data = mp_encode_uint(data, MP_ERROR_ERRNO);
> data = mp_encode_uint(data, error->saved_errno);
>
> if (is_client || is_access_denied || is_custom) {
> - data = mp_encode_uint(data, MP_ERROR_DET_CODE);
> + data = mp_encode_uint(data, MP_ERROR_CODE);
> data = mp_encode_uint(data, errcode);
> if (is_custom) {
> - data = mp_encode_uint(data, MP_ERROR_DET_CUSTOM_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_CUSTOM_TYPE);
> data = mp_encode_str(data, custom_type,
> strlen(custom_type));
> } else if (is_access_denied) {
> - data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_AD_OBJ_TYPE);
> data = mp_encode_str(data, ad_obj_type,
> strlen(ad_obj_type));
> - data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_NAME);
> + data = mp_encode_uint(data, MP_ERROR_AD_OBJ_NAME);
> data = mp_encode_str(data, ad_obj_name,
> strlen(ad_obj_name));
> - data = mp_encode_uint(data, MP_ERROR_DET_AD_ACCESS_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_AD_ACCESS_TYPE);
> data = mp_encode_str(data, ad_access_type,
> strlen(ad_access_type));
> }
> @@ -202,9 +182,9 @@ static struct error *
> build_error(struct mp_error *mp_error)
> {
> /*
> - * For create an error the "raw" constructor using
> - * because OOM error must be throwed in OOM case.
> - * Bulders returns a pointer to the static OOM error
> + * To create an error the "raw" constructor using
> + * because OOM error must be thrown in OOM case.
> + * Builders returns a pointer to the static OOM error
> * in OOM case.
> */
> struct error *err = NULL;
> @@ -270,8 +250,21 @@ build_error(struct mp_error *mp_error)
> return err;
> }
>
> +static inline const char *
> +region_strdup(struct region *region, const char *str, uint32_t len)
> +{
> + char *res = (char *)region_alloc(region, len + 1);
> + if (res == NULL) {
> + diag_set(OutOfMemory, len + 1, "region_alloc", "res");
> + return NULL;
> + }
> + memcpy(res, str, len);
> + res[len] = 0;
> + return res;
> +}
> +
> struct error *
> -error_unpack(const char **data, uint32_t len)
> +error_unpack_xc(const char **data, uint32_t len)
> {
> const char *end = *data + len;
> if (mp_typeof(**data) != MP_MAP) {
> @@ -281,8 +274,9 @@ error_unpack(const char **data, uint32_t len)
> }
>
> struct mp_error mp_err;
> - mp_error_init(&mp_err);
> -
> + mp_error_create(&mp_err);
> + struct region *region = &fiber()->gc;
> + uint32_t region_svp = region_used(region);
> uint32_t map_size = mp_decode_map(data);
>
> struct error *err = NULL;
> @@ -290,69 +284,87 @@ error_unpack(const char **data, uint32_t len)
> if (mp_typeof(**data) != MP_UINT) {
> diag_set(ClientError, ER_INVALID_MSGPACK,
> "Invalid MP_ERROR MsgPack format");
> - return NULL;
> + goto finish;
> }
>
> - uint8_t key = mp_decode_uint(data);
> + uint64_t key = mp_decode_uint(data);
> const char *str;
> uint32_t str_len;
> switch(key) {
> - case MP_ERROR_DET_TYPE:
> + case MP_ERROR_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.error_type = strndup(str, str_len);
> + mp_err.error_type = region_strdup(region, str, str_len);
> + if (mp_err.error_type == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_FILE:
> + case MP_ERROR_FILE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.file = strndup(str, str_len);
> + mp_err.file = region_strdup(region, str, str_len);
> + if (mp_err.file == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_LINE:
> + case MP_ERROR_LINE:
> if (mp_typeof(**data) != MP_UINT)
> goto error;
> mp_err.line = mp_decode_uint(data);
> break;
> - case MP_ERROR_DET_REASON:
> + case MP_ERROR_REASON:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.reason = strndup(str, str_len);
> + mp_err.reason = region_strdup(region, str, str_len);
> + if (mp_err.reason == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_ERRNO:
> + case MP_ERROR_ERRNO:
> if (mp_typeof(**data) != MP_UINT)
> goto error;
> mp_err.saved_errno = mp_decode_uint(data);
> break;
> - case MP_ERROR_DET_CODE:
> + case MP_ERROR_CODE:
> if (mp_typeof(**data) != MP_UINT)
> goto error;
> mp_err.error_code = mp_decode_uint(data);
> break;
> - case MP_ERROR_DET_CUSTOM_TYPE:
> + case MP_ERROR_CUSTOM_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.custom_type = strndup(str, str_len);
> + mp_err.custom_type = region_strdup(region, str,
> + str_len);
> + if (mp_err.custom_type == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_AD_OBJ_TYPE:
> + case MP_ERROR_AD_OBJ_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.ad_obj_type = strndup(str, str_len);
> + mp_err.ad_obj_type = region_strdup(region, str,
> + str_len);
> + if (mp_err.ad_obj_type == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_AD_OBJ_NAME:
> + case MP_ERROR_AD_OBJ_NAME:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.ad_obj_name = strndup(str, str_len);
> + mp_err.ad_obj_name = region_strdup(region, str,
> + str_len);
> + if (mp_err.ad_obj_name == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_AD_ACCESS_TYPE:
> + case MP_ERROR_AD_ACCESS_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.ad_access_type = strndup(str, str_len);
> + mp_err.ad_access_type = region_strdup(region, str,
> + str_len);
> + if (mp_err.ad_access_type == NULL)
> + goto finish;
> break;
> default:
> mp_next(data);
> @@ -363,11 +375,22 @@ error_unpack(const char **data, uint32_t len)
> assert(*data == end);
>
> err = build_error(&mp_err);
> - mp_error_cleanup(&mp_err);
> +finish:
> + region_truncate(region, region_svp);
> return err;
>
> error:
> diag_set(ClientError, ER_INVALID_MSGPACK,
> "Invalid MP_ERROR MsgPack format");
> - return NULL;
> + goto finish;
> +}
> +
> +struct error *
> +error_unpack(const char **data, uint32_t len)
> +{
> + try {
> + return error_unpack_xc(data, len);
> + } catch (OutOfMemory *e) {
> + return e;
> + }
> }
> diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
> index eb86bdc76..8a4f79c04 100755
> --- a/test/box-tap/extended_error.test.lua
> +++ b/test/box-tap/extended_error.test.lua
> @@ -3,12 +3,10 @@
> local netbox = require('net.box')
> local os = require('os')
> local tap = require('tap')
> -local uri = require('uri')
>
> local test = tap.test('check an extended error')
> test:plan(4)
>
> -
> function error_func()
> box.error(box.error.PROC_LUA, "An old good error")
> end
> @@ -22,22 +20,6 @@ function custom_error_func_2()
> return err
> end
>
> -local function version_at_least(peer_version_str, major, minor, patch)
> - local major_p, minor_p, patch_p = string.match(peer_version_str,
> - "(%d)%.(%d)%.(%d)")
> - major_p = tonumber(major_p)
> - minor_p = tonumber(minor_p)
> - patch_p = tonumber(patch_p)
> -
> - if major_p < major
> - or (major_p == major and minor_p < minor)
> - or (major_p == major and minor_p == minor and patch_p < patch) then
> - return false
> - end
> -
> - return true
> -end
> -
> local function grant_func(user, name)
> box.schema.func.create(name, {if_not_exists = true})
> box.schema.user.grant(user, 'execute', 'function', name, {
> @@ -85,15 +67,14 @@ local function check_error(err, check_list)
> return true
> end
>
> -local function test_old_transmission(host, port)
> +local function test_old_transmission()
> grant_func('guest', 'error_func')
> grant_func('guest', 'custom_error_func_2')
>
> - local connection = netbox.connect(host, port)
> + local connection = netbox.connect(box.cfg.listen)
> local _, err = pcall(connection.call, connection, 'error_func')
> local err_2 = connection:call('custom_error_func_2')
>
> -
> local check_list = {
> type = 'ClientError',
> message = 'An old good error',
> @@ -111,12 +92,12 @@ local function test_old_transmission(host, port)
> connection:close()
> end
>
> -local function test_extended_transmission(host, port)
> +local function test_extended_transmission()
> grant_func('guest', 'custom_error_func')
> grant_func('guest', 'custom_error_func_2')
> box.schema.user.grant('guest','read,write', 'space', '_session_settings')
>
> - local connection = netbox.connect(host, port)
> + local connection = netbox.connect(box.cfg.listen)
> connection.space._session_settings:update('error_marshaling_enabled',
> {{'=', 2, true}})
> local _, err = pcall(connection.call, connection, 'custom_error_func')
> @@ -148,10 +129,8 @@ end
> box.cfg{
> listen = os.getenv('LISTEN')
> }
> -local host= uri.parse(box.cfg.listen).host or 'localhost'
> -local port = uri.parse(box.cfg.listen).service
>
> -test_extended_transmission(host, port)
> -test_old_transmission(host, port)
> +test_extended_transmission()
> +test_old_transmission()
>
> os.exit(test:check() and 0 or 1)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Tarantool-patches] [PATCH V4 0/6] Extending error functionality
2020-04-16 17:38 [Tarantool-patches] [PATCH V4 0/6] Extending error functionality Leonid Vasiliev
` (5 preceding siblings ...)
2020-04-16 17:38 ` [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding Leonid Vasiliev
@ 2020-04-17 0:51 ` Vladislav Shpilevoy
6 siblings, 0 replies; 16+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-17 0:51 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches
Thanks for the patchset!
On 16/04/2020 19:38, Leonid Vasiliev wrote:
> https://github.com/tarantool/tarantool/issues/4398
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4398-expose-error-module-4
Can't build on Mac:
https://travis-ci.org/github/tarantool/tarantool/jobs/675863958
^ permalink raw reply [flat|nested] 16+ messages in thread