* [Tarantool-patches] [PATCH 0/6] Extending error functionality
@ 2020-03-24 12:45 Leonid Vasiliev
2020-03-24 12:45 ` [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error Leonid Vasiliev
` (7 more replies)
0 siblings, 8 replies; 30+ messages in thread
From: Leonid Vasiliev @ 2020-03-24 12:45 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
https://github.com/tarantool/tarantool/issues/4398
https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4398-expose-error-module-4
According to https://github.com/tarantool/tarantool/issues/4398
(and after some discussion) we would like box.error to have:
* Ability to create new error types
* Transparent marshalling through net.box
* Lua backtrace
* payload (not implemented in patch)
Leonid Vasiliev (6):
error: Add a Lua backtrace to error
error: Add the custom error type
iproto: Add negotiation phase
error: Add extended error transfer format
error: Add test for extended error
error: Transmit an error through IPROTO_OK as object
src/box/errcode.h | 1 +
src/box/error.cc | 56 +++++++++
src/box/error.h | 32 ++++++
src/box/execute.c | 1 +
src/box/iproto.cc | 40 ++++++-
src/box/iproto_constants.h | 25 +++++
src/box/lua/call.c | 29 +++--
src/box/lua/error.cc | 126 +++++++++++++++++----
src/box/lua/execute.c | 2 +-
src/box/lua/net_box.c | 34 ++++++
src/box/lua/net_box.lua | 103 +++++++++++++++--
src/box/lua/tuple.c | 12 +-
src/box/session.cc | 16 +++
src/box/session.h | 20 ++++
src/box/sql/func.c | 4 +-
src/box/xrow.c | 160 ++++++++++++++++++++++++++
src/box/xrow.h | 48 +++++++-
src/lib/core/diag.c | 20 ++++
src/lib/core/diag.h | 5 +
src/lib/core/exception.cc | 1 +
src/lib/core/mp_extension_types.h | 4 +-
src/lib/core/port.h | 4 +-
src/lua/error.c | 45 +++++++-
src/lua/error.h | 6 +-
src/lua/error.lua | 65 ++++++++---
src/lua/msgpack.c | 26 +++--
src/lua/msgpack.h | 8 +-
src/lua/utils.c | 28 ++++-
src/lua/utils.h | 27 ++++-
test/app/fiber.result | 35 ++++--
test/app/fiber.test.lua | 11 +-
test/box-tap/extended_error.test.lua | 212 +++++++++++++++++++++++++++++++++++
test/box/misc.result | 51 +++++++--
test/box/misc.test.lua | 19 +++-
34 files changed, 1161 insertions(+), 115 deletions(-)
create mode 100755 test/box-tap/extended_error.test.lua
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
@ 2020-03-24 12:45 ` Leonid Vasiliev
2020-04-05 22:14 ` Vladislav Shpilevoy
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 2/6] error: Add the custom error type Leonid Vasiliev
` (6 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Leonid Vasiliev @ 2020-03-24 12:45 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
Lua bactrace was added to a tarantool error.
In accordance with https://github.com/tarantool/tarantool/issues/4398
we want to have a Lua backtrace for the box.error
@TarantoolBot document
Title: error.bt
Lua backtrace was added to a tarantool error.
Needed for #4398
---
src/lib/core/diag.c | 20 ++++++++++++++++++++
src/lib/core/diag.h | 5 +++++
src/lib/core/exception.cc | 1 +
src/lua/error.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
src/lua/error.h | 3 +++
src/lua/error.lua | 20 +++++++++++++++-----
test/app/fiber.result | 35 +++++++++++++++++++++++++++--------
test/app/fiber.test.lua | 11 ++++++++++-
test/box/misc.result | 34 ++++++++++++++++++++++++++--------
test/box/misc.test.lua | 10 +++++++++-
10 files changed, 160 insertions(+), 24 deletions(-)
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index c350abb..c392c1e 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -53,6 +53,7 @@ error_create(struct error *e,
e->line = 0;
}
e->errmsg[0] = '\0';
+ e->lua_bt = NULL;
}
struct diag *
@@ -76,3 +77,22 @@ error_vformat_msg(struct error *e, const char *format, va_list ap)
vsnprintf(e->errmsg, sizeof(e->errmsg), format, ap);
}
+void
+error_set_lua_bt(struct error *e, const char *lua_bt)
+{
+ if (e == NULL)
+ return;
+
+ if (lua_bt == NULL) {
+ free(e->lua_bt);
+ e->lua_bt = NULL;
+ return;
+ }
+
+ size_t bt_len = strlen(lua_bt);
+ e->lua_bt = realloc(e->lua_bt, bt_len + 1);
+ if (e->lua_bt == NULL)
+ return;
+ strcpy(e->lua_bt, lua_bt);
+ return;
+}
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index f763957..c917871 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -84,6 +84,8 @@ struct error {
char file[DIAG_FILENAME_MAX];
/* Error description. */
char errmsg[DIAG_ERRMSG_MAX];
+ /* Lua backtrace */
+ char *lua_bt;
};
static inline void
@@ -126,6 +128,9 @@ error_format_msg(struct error *e, const char *format, ...);
void
error_vformat_msg(struct error *e, const char *format, va_list ap);
+void
+error_set_lua_bt(struct error *e, const char *lua_bt);
+
/**
* Diagnostics Area - a container for errors
*/
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index 76dcea5..d316d9b 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -42,6 +42,7 @@ extern "C" {
static void
exception_destroy(struct error *e)
{
+ free(e->lua_bt);
delete (Exception *) e;
}
diff --git a/src/lua/error.c b/src/lua/error.c
index d82e78d..3a12e20 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -34,8 +34,44 @@
#include <fiber.h>
#include "utils.h"
+#include <string.h>
+
static int CTID_CONST_STRUCT_ERROR_REF = 0;
+/*
+ * Memory for the traceback string is obtained with malloc,
+ * and can be freed with free.
+*/
+static char*
+traceback (lua_State *L) {
+ int top = lua_gettop(L);
+
+ lua_getfield(L, LUA_GLOBALSINDEX, "debug");
+ if (!lua_istable(L, -1)) {
+ lua_settop(L, top);
+ return NULL;
+ }
+ lua_getfield(L, -1, "traceback");
+ if (!lua_isfunction(L, -1)) {
+ lua_settop(L, top);
+ return NULL;
+ }
+
+ // call debug.traceback
+ lua_call(L, 0, 1);
+
+ // get result of the debug.traceback call
+ if (!lua_isstring(L, -1)) {
+ lua_settop(L, top);
+ return NULL;
+ }
+
+ char *bt = strdup(lua_tostring(L, -1));
+ lua_settop(L, top);
+
+ return bt;
+}
+
struct error *
luaL_iserror(struct lua_State *L, int narg)
{
@@ -53,7 +89,7 @@ luaL_iserror(struct lua_State *L, int narg)
return e;
}
-static struct error *
+struct error *
luaL_checkerror(struct lua_State *L, int narg)
{
struct error *error = luaL_iserror(L, narg);
@@ -85,6 +121,13 @@ luaT_pusherror(struct lua_State *L, struct error *e)
* then set the finalizer.
*/
error_ref(e);
+
+ if (e->lua_bt == NULL) {
+ char *lua_bt = traceback(L);
+ error_set_lua_bt(e, lua_bt);
+ free(lua_bt);
+ }
+
assert(CTID_CONST_STRUCT_ERROR_REF != 0);
struct error **ptr = (struct error **)
luaL_pushcdata(L, CTID_CONST_STRUCT_ERROR_REF);
diff --git a/src/lua/error.h b/src/lua/error.h
index 64fa5eb..16cdaf7 100644
--- a/src/lua/error.h
+++ b/src/lua/error.h
@@ -65,6 +65,9 @@ luaT_pusherror(struct lua_State *L, struct error *e);
struct error *
luaL_iserror(struct lua_State *L, int narg);
+struct error *
+luaL_checkerror(struct lua_State *L, int narg);
+
void
tarantool_lua_error_init(struct lua_State *L);
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 7f24986..765ce73 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -24,6 +24,7 @@ struct error {
char _file[DIAG_FILENAME_MAX];
/* Error description. */
char _errmsg[DIAG_ERRMSG_MAX];
+ char *lua_bt;
};
char *
@@ -83,10 +84,18 @@ local function error_trace(err)
return {}
end
return {
- { file = ffi.string(err._file), line = tonumber(err._line) };
+ { file = ffi.string(err._file), line = tonumber(err._line) }
}
end
+local function error_backtrace(err)
+ local result = "Backtrace is absent"
+ if err.lua_bt ~= ffi.nullptr then
+ result = ffi.string(err.lua_bt)
+ end
+ return result
+end
+
local function error_errno(err)
local e = err._saved_errno
if e == 0 then
@@ -96,10 +105,11 @@ local function error_errno(err)
end
local error_fields = {
- ["type"] = error_type;
- ["message"] = error_message;
- ["trace"] = error_trace;
- ["errno"] = error_errno;
+ ["type"] = error_type,
+ ["message"] = error_message,
+ ["trace"] = error_trace,
+ ["errno"] = error_errno,
+ ["bt"] = error_backtrace
}
local function error_unpack(err)
diff --git a/test/app/fiber.result b/test/app/fiber.result
index 7331f61..3908733 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1036,14 +1036,33 @@ st;
---
- false
...
-e:unpack();
----
-- type: ClientError
- code: 1
- message: Illegal parameters, oh my
- trace:
- - file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
- line: 1
+unpack_res = e:unpack();
+---
+...
+unpack_res['code'] == 1;
+---
+- true
+...
+unpack_res['trace'][1]['file'] == '[string "function err()' ..
+ ' box.error(box.error.ILLEGAL_PA..."]';
+---
+- true
+...
+unpack_res['trace'][1]['line'] == 1;
+---
+- true
+...
+unpack_res['type'] == 'ClientError';
+---
+- true
+...
+unpack_res['message'] == 'Illegal parameters, oh my';
+---
+- true
+...
+unpack_res['bt'] == e.bt;
+---
+- true
...
flag = false;
---
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index b8e9abc..f3616ec 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -428,7 +428,16 @@ function test1()
end;
st, e = test1();
st;
-e:unpack();
+
+unpack_res = e:unpack();
+
+unpack_res['code'] == 1;
+unpack_res['trace'][1]['file'] == '[string "function err()' ..
+ ' box.error(box.error.ILLEGAL_PA..."]';
+unpack_res['trace'][1]['line'] == 1;
+unpack_res['type'] == 'ClientError';
+unpack_res['message'] == 'Illegal parameters, oh my';
+unpack_res['bt'] == e.bt;
flag = false;
function test2()
diff --git a/test/box/misc.result b/test/box/misc.result
index 047591b..f432d51 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -125,14 +125,32 @@ e
---
- Illegal parameters, bla bla
...
-e:unpack()
----
-- type: ClientError
- code: 1
- message: Illegal parameters, bla bla
- trace:
- - file: '[C]'
- line: 4294967295
+unpack_res = e:unpack()
+---
+...
+unpack_res['code'] == 1
+---
+- true
+...
+unpack_res['trace'][1]['file'] == '[C]'
+---
+- true
+...
+unpack_res['trace'][1]['line'] == 4294967295
+---
+- true
+...
+unpack_res['type'] == 'ClientError'
+---
+- true
+...
+unpack_res['message'] == 'Illegal parameters, bla bla'
+---
+- true
+...
+unpack_res['bt'] == e.bt
+---
+- true
...
e.type
---
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index e1c2f99..e82ac2c 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -35,7 +35,15 @@ box.error(box.error.ILLEGAL_PARAMS, "bla bla")
box.error()
e = box.error.last()
e
-e:unpack()
+
+unpack_res = e:unpack()
+unpack_res['code'] == 1
+unpack_res['trace'][1]['file'] == '[C]'
+unpack_res['trace'][1]['line'] == 4294967295
+unpack_res['type'] == 'ClientError'
+unpack_res['message'] == 'Illegal parameters, bla bla'
+unpack_res['bt'] == e.bt
+
e.type
e.code
e.message
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Tarantool-patches] [PATCH 2/6] error: Add the custom error type
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
2020-03-24 12:45 ` [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error Leonid Vasiliev
@ 2020-03-24 12:46 ` Leonid Vasiliev
2020-04-05 22:14 ` Vladislav Shpilevoy
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase Leonid Vasiliev
` (5 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Leonid Vasiliev @ 2020-03-24 12:46 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
A possibility to create an error with a custom subtype was added.
In accordance with https://github.com/tarantool/tarantool/issues/4398
a custom error type has been added to the box.error.
Now, it's possible to create an error with a custom subtype (string value)
for use it in applications.
@TarantoolBot document
Title: error.custom_type
A custom error type has been added to the box.error.
Now, it's possible to create an error with a custom subtype (string value)
for use it in applications.
Example:
err_custom = box.error.new(box.error.CUSTOM_ERROR, "My Custom Type", "Reason")
Now:
err_custom.type == "CustomError"
err_custom.custom_type == "My Custom Type"
err_custom.message == "User custom error: Reason"
Needed for #4398
---
src/box/errcode.h | 1 +
src/box/error.cc | 56 ++++++++++++++++++++++++++++++
src/box/error.h | 32 ++++++++++++++++++
src/box/lua/error.cc | 92 ++++++++++++++++++++++++++++++++++++++------------
src/lua/error.lua | 25 +++++++++-----
test/box/misc.result | 19 +++++++++++
test/box/misc.test.lua | 9 +++++
7 files changed, 204 insertions(+), 30 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 4441717..a2f23dd 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -265,6 +265,7 @@ struct errcode_record {
/*210 */_(ER_SQL_PREPARE, "Failed to prepare SQL statement: %s") \
/*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_CUSTOM_ERROR, "User 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 47dce33..25e7eff 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -45,6 +45,16 @@ box_error_type(const box_error_t *e)
return e->type->name;
}
+const char *
+box_custom_error_type(const box_error_t *e)
+{
+ CustomError *custom_error = type_cast(CustomError, e);
+ if (custom_error)
+ return custom_error->custom_type();
+
+ return NULL;
+}
+
uint32_t
box_error_code(const box_error_t *e)
{
@@ -86,6 +96,17 @@ box_error_set(const char *file, unsigned line, uint32_t code,
return -1;
}
+int
+box_custom_error_set(const char *file, unsigned line,
+ const char *custom, const char *reason)
+{
+ struct error *e = BuildCustomError(file, line, custom);
+ strncpy(e->errmsg, reason, DIAG_ERRMSG_MAX);
+ e->errmsg[DIAG_ERRMSG_MAX - 1] = '\0';
+ diag_add_error(&fiber()->diag, e);
+ return -1;
+}
+
/* }}} */
struct rmean *rmean_error = NULL;
@@ -253,3 +274,38 @@ 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 ?: "");
+
+ if (custom_type) {
+ strncpy(m_custom_type, custom_type, 63);
+ m_custom_type[63] = '\0';
+ } else {
+ m_custom_type[0] = '\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 b8c7cf7..3e0beb8 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;
@@ -70,6 +73,14 @@ const char *
box_error_type(const box_error_t *error);
/**
+ * Return the error custom type,
+ * \param error
+ * \return pointer to custom error type. On error, return NULL
+ */
+const char *
+box_custom_error_type(const box_error_t *e);
+
+/**
* Return IPROTO error code
* \param error error
* \return enum box_error_code
@@ -129,6 +140,10 @@ int
box_error_set(const char *file, unsigned line, uint32_t code,
const char *format, ...);
+int
+box_custom_error_set(const char *file, unsigned line,
+ const char *custom, const char *reason);
+
/**
* A backward-compatible API define.
*/
@@ -140,6 +155,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
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" */
@@ -266,6 +282,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 fc53a40..708d338 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -43,31 +43,35 @@ extern "C" {
#include "box/error.h"
static void
-luaT_error_create(lua_State *L, int top_base)
-{
- uint32_t code = 0;
- const char *reason = NULL;
- const char *file = "";
- unsigned line = 0;
- lua_Debug info;
+luaT_error_read_args(lua_State *L, int top_base, uint32_t *code,
+ const char **custom_type, const char **reason) {
int top = lua_gettop(L);
if (top >= top_base && lua_type(L, top_base) == LUA_TNUMBER) {
- code = lua_tonumber(L, top_base);
- reason = tnt_errcode_desc(code);
+ *code = lua_tonumber(L, top_base);
+ *reason = tnt_errcode_desc(*code);
if (top > top_base) {
+ int shift = 1;
+ if (*code == ER_CUSTOM_ERROR) {
+ *custom_type = lua_tostring(L, top_base + 1);
+ shift = 2;
+ }
+
/* Call string.format(reason, ...) to format message */
lua_getglobal(L, "string");
if (lua_isnil(L, -1))
- goto raise;
+ return;
lua_getfield(L, -1, "format");
if (lua_isnil(L, -1))
- goto raise;
- lua_pushstring(L, reason);
- for (int i = top_base + 1; i <= top; i++)
+ return;
+
+ lua_pushstring(L, *reason);
+ int nargs = 1;
+ for (int i = top_base + shift; i <= top; ++i, ++nargs)
lua_pushvalue(L, i);
- lua_call(L, top - top_base + 1, 1);
- reason = lua_tostring(L, -1);
- } else if (strchr(reason, '%') != NULL) {
+
+ lua_call(L, nargs, 1);
+ *reason = lua_tostring(L, -1);
+ } else if (strchr(*reason, '%') != NULL) {
/* Missing arguments to format string */
luaL_error(L, "box.error(): bad arguments");
}
@@ -75,12 +79,15 @@ luaT_error_create(lua_State *L, int top_base)
if (lua_istable(L, top_base)) {
/* A special case that rethrows raw error (used by net.box) */
lua_getfield(L, top_base, "code");
- code = lua_tonumber(L, -1);
+ *code = lua_tonumber(L, -1);
lua_pop(L, 1);
+ if (*code == ER_CUSTOM_ERROR) {
+ lua_getfield(L, top_base, "custom_type");
+ *custom_type = lua_tostring(L, -1);
+ lua_pop(L, 1);
+ }
lua_getfield(L, top_base, "reason");
- reason = lua_tostring(L, -1);
- if (reason == NULL)
- reason = "";
+ *reason = lua_tostring(L, -1);
lua_pop(L, 1);
} else if (luaL_iserror(L, top_base)) {
lua_error(L);
@@ -90,7 +97,21 @@ luaT_error_create(lua_State *L, int top_base)
luaL_error(L, "box.error(): bad arguments");
}
-raise:
+ return;
+}
+
+static void
+luaT_error_create(lua_State *L, int top_base)
+{
+ uint32_t code = 0;
+ const char *custom_type = NULL;
+ const char *reason = NULL;
+
+ luaT_error_read_args(L, top_base, &code, &custom_type, &reason);
+
+ const char *file = "";
+ unsigned line = 0;
+ lua_Debug info;
if (lua_getstack(L, 1, &info) && lua_getinfo(L, "Sl", &info)) {
if (*info.short_src) {
file = info.short_src;
@@ -101,8 +122,16 @@ raise:
}
line = info.currentline;
}
+
+ if (reason == NULL)
+ reason = "";
say_debug("box.error() at %s:%i", file, line);
- box_error_set(file, line, code, "%s", reason);
+ if (code == ER_CUSTOM_ERROR) {
+ box_custom_error_set(file, line,
+ custom_type ? custom_type : "", reason);
+ } else {
+ box_error_set(file, line, code, "%s", reason);
+ }
}
static int
@@ -145,6 +174,21 @@ luaT_error_new(lua_State *L)
}
static int
+luaT_error_custom_type(lua_State *L)
+{
+ struct error *e = luaL_checkerror(L, -1);
+
+ const char *custom_type = box_custom_error_type(e);
+ if (custom_type == NULL) {
+ lua_pushfstring(L, "The error has't a custom type");
+ return 1;
+ }
+
+ lua_pushstring(L, custom_type);
+ return 1;
+}
+
+static int
luaT_error_clear(lua_State *L)
{
if (lua_gettop(L) >= 1)
@@ -268,6 +312,10 @@ box_lua_error_init(struct lua_State *L) {
lua_pushcfunction(L, luaT_error_new);
lua_setfield(L, -2, "new");
}
+ {
+ lua_pushcfunction(L, luaT_error_custom_type);
+ lua_setfield(L, -2, "custom_type");
+ }
lua_setfield(L, -2, "__index");
}
lua_setmetatable(L, -2);
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 765ce73..26abec8 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -104,6 +104,10 @@ local function error_errno(err)
return e
end
+local function error_custom_type(err)
+ return box.error.custom_type(err)
+end
+
local error_fields = {
["type"] = error_type,
["message"] = error_message,
@@ -148,11 +152,16 @@ local function error_serialize(err)
return error_message(err)
end
+local function error_is_custom(err)
+ return ffi.string(err._type.name) == 'CustomError'
+end
+
local error_methods = {
- ["unpack"] = error_unpack;
- ["raise"] = error_raise;
- ["match"] = error_match; -- Tarantool 1.6 backward compatibility
- ["__serialize"] = error_serialize;
+ ["unpack"] = error_unpack,
+ ["raise"] = error_raise,
+ ["match"] = error_match, -- Tarantool 1.6 backward compatibility
+ ["is_custom"] = error_is_custom,
+ ["__serialize"] = error_serialize
}
local function error_index(err, key)
@@ -181,11 +190,11 @@ local function error_concat(lhs, rhs)
end
local error_mt = {
- __index = error_index;
- __tostring = error_message;
- __concat = error_concat;
+ __index = error_index,
+ __tostring = error_message,
+ __concat = error_concat
};
-ffi.metatype('struct error', error_mt);
+ffi.metatype('struct error', error_mt)
return error
diff --git a/test/box/misc.result b/test/box/misc.result
index f432d51..20d01ef 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -308,6 +308,24 @@ type(err.errno)
---
- nil
...
+--
+-- gh-4398-expose-error-module
+--
+err_custom = box.error.new(box.error.CUSTOM_ERROR, "My Custom Type", "Reason")
+---
+...
+err_custom.type == "CustomError"
+---
+- true
+...
+err_custom.custom_type == "My Custom Type"
+---
+- true
+...
+err_custom.message == "User custom error: Reason"
+---
+- true
+...
----------------
-- # box.stat
----------------
@@ -655,6 +673,7 @@ t;
210: box.error.SQL_PREPARE
211: box.error.WRONG_QUERY_ID
212: box.error.SEQUENCE_NOT_STARTED
+ 213: box.error.CUSTOM_ERROR
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index e82ac2c..92b3a12 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -99,6 +99,15 @@ type(err.errno)
err = box.error.new(box.error.PROC_LUA, "errno")
type(err.errno)
+--
+-- gh-4398-expose-error-module
+--
+
+err_custom = box.error.new(box.error.CUSTOM_ERROR, "My Custom Type", "Reason")
+err_custom.type == "CustomError"
+err_custom.custom_type == "My Custom Type"
+err_custom.message == "User custom error: Reason"
+
----------------
-- # box.stat
----------------
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
2020-03-24 12:45 ` [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error Leonid Vasiliev
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 2/6] error: Add the custom error type Leonid Vasiliev
@ 2020-03-24 12:46 ` Leonid Vasiliev
2020-03-24 20:02 ` Konstantin Osipov
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 4/6] error: Add extended error transfer format Leonid Vasiliev
` (4 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Leonid Vasiliev @ 2020-03-24 12:46 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
The negotiation phase has been added to IPROTO
For possibility to have a custom parameters of session the negotiation
phase has been added. This is necessary to enable the transmission of
an error in different formats(depending on the choice of the client).
@TarantoolBot document
Title: IPROTO: The negatiation phase
For backward compatibility of the data transmission format,
the negotiation phase has been added to IPROTO.
A new key (IPROTO_NEGOTIATION) has been added to IPROTO command codes.
NEGOTIATION BODY: CODE = 0x0E
+==========================+
| |
| NEGOTIATION PARAMETERS |
| |
+==========================+
MP_MAP
Session negotiation parameters are a map with keys like ERROR_FORMAT_VERSION ...
The response is a map with all stated negotiation parameters.
So, for work with the new format of errors, it is necessary to perform the negotiation phase,
otherwise errors will be transmitted in the old format (by default).
Needed for #4398
---
src/box/iproto.cc | 20 +++++++++++
src/box/iproto_constants.h | 11 ++++++
src/box/lua/net_box.c | 34 ++++++++++++++++++
src/box/lua/net_box.lua | 57 ++++++++++++++++++++++++++----
src/box/session.cc | 12 +++++++
src/box/session.h | 27 ++++++++++++++
src/box/xrow.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++
src/box/xrow.h | 38 ++++++++++++++++++++
8 files changed, 279 insertions(+), 7 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 9dad43b..2df41ba 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -197,6 +197,8 @@ struct iproto_msg
struct auth_request auth;
/* SQL request, if this is the EXECUTE/PREPARE request. */
struct sql_request sql;
+ /** Negotiation request */
+ struct negotiation_params neg_req;
/** In case of iproto parse error, saved diagnostics. */
struct diag diag;
};
@@ -1309,6 +1311,16 @@ iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend,
goto error;
cmsg_init(&msg->base, misc_route);
break;
+ case IPROTO_NEGOTIATION: {
+ struct session *ses = msg->connection->session;
+ /* Copy current parameters to modify */
+ memcpy(&msg->neg_req, &ses->neg_param,
+ sizeof(struct negotiation_params));
+ if (xrow_decode_negotiation(&msg->header, &msg->neg_req))
+ goto error;
+ cmsg_init(&msg->base, misc_route);
+ break;
+ }
default:
diag_set(ClientError, ER_UNKNOWN_REQUEST_TYPE,
(uint32_t) type);
@@ -1714,6 +1726,14 @@ tx_process_misc(struct cmsg *m)
iproto_reply_vote_xc(out, &ballot, msg->header.sync,
::schema_version);
break;
+ case IPROTO_NEGOTIATION:
+ session_update_neg_parameters(con->session,
+ &msg->neg_req);
+ iproto_reply_negotiation_xc(out,
+ &con->session->neg_param,
+ msg->header.sync,
+ ::schema_version);
+ break;
default:
unreachable();
}
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index f9d413a..1851712 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -112,6 +112,7 @@ enum iproto_key {
IPROTO_METADATA = 0x32,
IPROTO_BIND_METADATA = 0x33,
IPROTO_BIND_COUNT = 0x34,
+ IPROTO_NEG_PARAM = 0x35,
/* Leave a gap between response keys and SQL keys. */
IPROTO_SQL_TEXT = 0x40,
@@ -215,6 +216,8 @@ enum iproto_type {
IPROTO_NOP = 12,
/** Prepare SQL statement. */
IPROTO_PREPARE = 13,
+ /** Negotiation of session parameters */
+ IPROTO_NEGOTIATION = 14,
/** The maximum typecode used for box.stat() */
IPROTO_TYPE_STAT_MAX,
@@ -467,6 +470,14 @@ vy_row_index_key_name(enum vy_row_index_key key)
return vy_row_index_key_strs[key];
}
+/**
+ * Negotiation protocol's keys
+ */
+enum neg_key {
+ /** Version of an error format */
+ ERROR_FORMAT_VERSION
+};
+
#if defined(__cplusplus)
} /* extern "C" */
#endif
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index c7bd016..5321e87 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -417,6 +417,39 @@ netbox_encode_upsert(lua_State *L)
}
static int
+netbox_encode_negotiation(lua_State *L)
+{
+ if (lua_gettop(L) < 3)
+ return luaL_error(L, "Usage: netbox.encode_negotiation(ibuf, sync, "
+ "opts)");
+
+
+ /* Check opts is table and parse it */
+ if (lua_istable(L, 3) == 0) {
+ return luaL_error(L, "Expected opts is table");
+ }
+
+ lua_getfield(L, 3, "error_format_ver");
+
+ int err_format_ver = ERR_FORMAT_DEF;
+ if (lua_isnumber(L, -1)) {
+ err_format_ver = lua_tonumber(L, -1);
+ }
+
+ struct mpstream stream;
+ size_t svp = netbox_prepare_request(L, &stream, IPROTO_NEGOTIATION);
+ netbox_encode_request(&stream, svp);
+
+ mpstream_encode_map(&stream, 1);
+ mpstream_encode_uint(&stream, ERROR_FORMAT_VERSION);
+ mpstream_encode_uint(&stream, err_format_ver);
+
+ netbox_encode_request(&stream, svp);
+
+ return 0;
+}
+
+static int
netbox_decode_greeting(lua_State *L)
{
struct greeting greeting;
@@ -901,6 +934,7 @@ luaopen_net_box(struct lua_State *L)
{ "encode_execute", netbox_encode_execute},
{ "encode_prepare", netbox_encode_prepare},
{ "encode_auth", netbox_encode_auth },
+ { "encode_negotiation", netbox_encode_negotiation },
{ "decode_greeting",netbox_decode_greeting },
{ "communicate", netbox_communicate },
{ "decode_select", netbox_decode_select },
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 3f611c0..7fdad64 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -26,6 +26,7 @@ local check_primary_index = box.internal.check_primary_index
local communicate = internal.communicate
local encode_auth = internal.encode_auth
local encode_select = internal.encode_select
+local encode_negotiation = internal.encode_negotiation
local decode_greeting = internal.decode_greeting
local TIMEOUT_INFINITY = 500 * 365 * 86400
@@ -38,12 +39,13 @@ local IPROTO_STATUS_KEY = 0x00
local IPROTO_ERRNO_MASK = 0x7FFF
local IPROTO_SYNC_KEY = 0x01
local IPROTO_SCHEMA_VERSION_KEY = 0x05
-local IPROTO_METADATA_KEY = 0x32
-local IPROTO_SQL_INFO_KEY = 0x42
+local IPROTO_METADATA_KEY = 0x32
+local IPROTO_SQL_INFO_KEY = 0x42
local SQL_INFO_ROW_COUNT_KEY = 0
local IPROTO_FIELD_NAME_KEY = 0
local IPROTO_DATA_KEY = 0x30
local IPROTO_ERROR_KEY = 0x31
+local IPROTO_NEG_PARAM = 0x35
local IPROTO_GREETING_SIZE = 128
local IPROTO_CHUNK_KEY = 128
local IPROTO_OK_KEY = 0
@@ -57,6 +59,11 @@ local E_PROC_LUA = box.error.PROC_LUA
-- utility tables
local is_final_state = {closed = 1, error = 1}
+-- negotiations keys
+local neg_keys = {
+ ERROR_FORMAT_VERSION = 0
+}
+
local function decode_nil(raw_data, raw_data_end)
return nil, raw_data_end
end
@@ -83,6 +90,10 @@ local function decode_push(raw_data)
local response, raw_end = decode(raw_data)
return response[IPROTO_DATA_KEY][1], raw_end
end
+local function decode_negotiation(raw_data)
+ local response, raw_end = decode(raw_data)
+ return response[IPROTO_NEG_PARAM], raw_end
+end
local function version_id(major, minor, patch)
return bit.bor(bit.lshift(major, 16), bit.lshift(minor, 8), patch)
@@ -110,6 +121,7 @@ local method_encoder = {
min = internal.encode_select,
max = internal.encode_select,
count = internal.encode_call,
+ negotiation = internal.encode_negotiation,
-- inject raw data into connection, used by console and tests
inject = function(buf, id, bytes)
local ptr = buf:reserve(#bytes)
@@ -138,9 +150,12 @@ local method_decoder = {
count = decode_count,
inject = decode_data,
push = decode_push,
+ negotiation = decode_negotiation
}
-local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
+local function next_id(id)
+ return band(id + 1, 0x7FFFFFFF)
+end
--
-- Connect to a remote server, do handshake.
@@ -435,7 +450,9 @@ local function create_transport(host, port, user, password, callback,
local protocol_sm
local function start()
- if state ~= 'initial' then return not is_final_state[state] end
+ if state ~= 'initial' then
+ return not is_final_state[state]
+ end
fiber.create(function()
local ok, err, timeout
worker_fiber = fiber_self()
@@ -804,7 +821,9 @@ local function create_transport(host, port, user, password, callback,
iproto_sm = function(schema_version)
local err, hdr, body_rpos, body_end = send_and_recv_iproto()
- if err then return error_sm(err, hdr) end
+ if err then
+ return error_sm(err, hdr)
+ end
dispatch_response_iproto(hdr, body_rpos, body_end)
local status = hdr[IPROTO_STATUS_KEY]
local response_schema_version = hdr[IPROTO_SCHEMA_VERSION_KEY]
@@ -821,7 +840,10 @@ local function create_transport(host, port, user, password, callback,
end
error_sm = function(err, msg)
- if connection then connection:close(); connection = nil end
+ if connection then
+ connection:close()
+ connection = nil
+ end
send_buf:recycle()
recv_buf:recycle()
if state ~= 'closed' then
@@ -924,8 +946,11 @@ local console_mt = {
local space_metatable, index_metatable
+
+
local function new_sm(host, port, opts, connection, greeting)
- local user, password = opts.user, opts.password; opts.password = nil
+ local user, password = opts.user, opts.password
+ opts.password = nil
local last_reconnect_error
local remote = {host = host, port = port, opts = opts, state = 'initial'}
local function callback(what, ...)
@@ -1004,6 +1029,24 @@ local function new_sm(host, port, opts, connection, greeting)
if opts.wait_connected ~= false then
remote._transport.wait_state('active', tonumber(opts.wait_connected))
end
+
+ -- Negotiation if needed
+ if opts.error_extended then
+ local peer_has_negotiation = version_at_least(remote.peer_version_id, 2, 4, 1)
+ if not peer_has_negotiation then
+ box.error(box.error.PROC_LUA, "Negotiations failed")
+ end
+ local neg_req = {error_format_ver = 1}
+ local neg_resp = remote:_request("negotiation", nil, nil, neg_req)
+ if neg_resp[neg_keys.ERROR_FORMAT_VERSION] ~= 1 then
+ remote._transport.stop()
+ box.error(box.error.PROC_LUA, "Negotiations failed")
+ end
+
+ remote._error_extended = true
+ end
+ -- Negotiation end
+
return remote
end
diff --git a/src/box/session.cc b/src/box/session.cc
index 8813182..c0224ca 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -37,6 +37,7 @@
#include "error.h"
#include "tt_static.h"
#include "sql_stmt_cache.h"
+#include "iproto_constants.h"
const char *session_type_strs[] = {
"background",
@@ -144,6 +145,9 @@ session_create(enum session_type type)
session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX;
session->sql_stmts = NULL;
+ /* Set default negotiation parameters */
+ session->neg_param.err_format_ver = ERR_FORMAT_DEF;
+
/* For on_connect triggers. */
credentials_create(&session->credentials, guest_user);
struct mh_i64ptr_node_t node;
@@ -373,3 +377,11 @@ generic_session_sync(struct session *session)
(void) session;
return 0;
}
+
+int
+session_update_neg_parameters(struct session *session,
+ const struct negotiation_params *params)
+{
+ session->neg_param.err_format_ver = params->err_format_ver;
+ return 0;
+}
diff --git a/src/box/session.h b/src/box/session.h
index 6dfc7cb..3b4dfb0 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -81,6 +81,24 @@ union session_meta {
};
/**
+ * An error transmission formats
+ */
+enum error_formats {
+ /** Default(old) format */
+ ERR_FORMAT_DEF,
+ /** Extended format */
+ ERR_FORMAT_EX
+};
+
+/**
+ * Parameters which may be changed at negotiation phase of session
+*/
+struct negotiation_params {
+ /** Version of a format for an error transmission */
+ uint8_t err_format_ver;
+};
+
+/**
* Abstraction of a single user session:
* for now, only provides accounting of established
* sessions and on-connect/on-disconnect event
@@ -110,6 +128,8 @@ struct session {
struct credentials credentials;
/** Trigger for fiber on_stop to cleanup created on-demand session */
struct trigger fiber_on_stop;
+ /** Negotiation parameters */
+ struct negotiation_params neg_param;
};
struct session_vtab {
@@ -364,6 +384,13 @@ generic_session_fd(struct session *session);
int64_t
generic_session_sync(struct session *session);
+/**
+ * Update negatiation parameters of the session
+*/
+int
+session_update_neg_parameters(struct session *session,
+ const struct negotiation_params *params);
+
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 5e3cb07..90dffea 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -479,6 +479,42 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot,
}
int
+iproto_reply_negotiation(struct obuf *out,
+ const struct negotiation_params *neg_param,
+ uint64_t sync,
+ uint32_t schema_version)
+{
+ char *buf = NULL;
+ uint32_t buf_size = IPROTO_HEADER_LEN + mp_sizeof_map(1)
+ + mp_sizeof_uint(IPROTO_NEG_PARAM) + mp_sizeof_map(1)
+ + mp_sizeof_uint(ERROR_FORMAT_VERSION)
+ + mp_sizeof_uint(neg_param->err_format_ver);
+
+ buf = obuf_reserve(out, buf_size);
+ if (buf == NULL) {
+ diag_set(OutOfMemory, buf_size,
+ "obuf_alloc", "buf");
+ return -1;
+ };
+
+ char *data = buf + IPROTO_HEADER_LEN;
+ data = mp_encode_map(data, 1);
+ data = mp_encode_uint(data, IPROTO_NEG_PARAM);
+ data = mp_encode_map(data, 1);
+ data = mp_encode_uint(data, ERROR_FORMAT_VERSION);
+ data = mp_encode_uint(data, neg_param->err_format_ver);
+ assert(data == buf + buf_size);
+
+ iproto_header_encode(buf, IPROTO_OK, sync, schema_version,
+ buf_size - IPROTO_HEADER_LEN);
+
+ char *ptr = obuf_alloc(out, buf_size);
+ (void) ptr;
+ assert(ptr == buf);
+ return 0;
+}
+
+int
iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
uint32_t schema_version)
{
@@ -1518,3 +1554,54 @@ greeting_decode(const char *greetingbuf, struct greeting *greeting)
return 0;
}
+
+int
+xrow_decode_negotiation(const struct xrow_header *row,
+ struct negotiation_params *request)
+{
+ if (row->bodycnt == 0) {
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "missing request body");
+ return -1;
+ }
+
+ assert(row->bodycnt == 1);
+ const char *data = (const char *) row->body[0].iov_base;
+
+ const char *end = data + row->body[0].iov_len;
+ assert((end - data) > 0);
+
+ if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
+error:
+ xrow_on_decode_err(row->body[0].iov_base,
+ end, ER_INVALID_MSGPACK,
+ "packet body");
+ return -1;
+ }
+
+ uint32_t map_size = mp_decode_map(&data);
+ for (uint32_t i = 0; i < map_size; ++i) {
+ if ((end - data) < 1 || mp_typeof(*data) != MP_UINT)
+ goto error;
+
+ uint64_t key = mp_decode_uint(&data);
+
+ switch (key) {
+ case ERROR_FORMAT_VERSION:
+ if (mp_typeof(*data) != MP_UINT)
+ goto error;
+ request->err_format_ver = mp_decode_uint(&data);
+ break;
+ default:
+ /* unknown key */
+ continue;
+ }
+ }
+ if (data != end) {
+ xrow_on_decode_err(row->body[0].iov_base, end,
+ ER_INVALID_MSGPACK, "packet end");
+ return -1;
+ }
+
+ return 0;
+}
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 2a0a9c8..bbd0496 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -37,6 +37,7 @@
#include "uuid/tt_uuid.h"
#include "diag.h"
+#include "session.h"
#include "vclock.h"
#if defined(__cplusplus)
@@ -177,6 +178,17 @@ struct request {
};
/**
+ * Decode negotiation request
+ * @param row
+ * @param request
+ * @retval -1 on error, see diag
+ * @retval 0 success
+ */
+int
+xrow_decode_negotiation(const struct xrow_header *row,
+ struct negotiation_params *request);
+
+/**
* Create a JSON-like string representation of a request. */
const char *
request_str(const struct request *request);
@@ -566,6 +578,22 @@ int
iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
uint32_t schema_version);
+/**
+ * Write a negotiation reply packet to output buffer.
+ * @param out Buffer to write to.
+ * @param neg_param Current negotiation parameters.
+ * @param sync Request sync.
+ * @param schema_version Actual schema version.
+ *
+ * @retval 0 Success.
+ * @retval -1 Memory error.
+ */
+int
+iproto_reply_negotiation(struct obuf *out,
+ const struct negotiation_params *neg_param,
+ uint64_t sync,
+ uint32_t schema_version);
+
/** EXECUTE/PREPARE request. */
struct sql_request {
/** SQL statement text. */
@@ -936,6 +964,16 @@ iproto_reply_vote_xc(struct obuf *out, const struct ballot *ballot,
diag_raise();
}
+/** @copydoc iproto_reply_negotiation. */
+static inline void
+iproto_reply_negotiation_xc(struct obuf *out,
+ const struct negotiation_params *neg_param,
+ uint64_t sync, uint32_t schema_version)
+{
+ if (iproto_reply_negotiation(out, neg_param, sync, schema_version) != 0)
+ diag_raise();
+}
+
#endif
#endif /* TARANTOOL_XROW_H_INCLUDED */
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Tarantool-patches] [PATCH 4/6] error: Add extended error transfer format
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
` (2 preceding siblings ...)
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase Leonid Vasiliev
@ 2020-03-24 12:46 ` Leonid Vasiliev
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 5/6] error: Add test for extended error Leonid Vasiliev
` (3 subsequent siblings)
7 siblings, 0 replies; 30+ messages in thread
From: Leonid Vasiliev @ 2020-03-24 12:46 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
Extended error transfer format (through IPROTO_ERROR) has been added.
Extended error transfer format (through IPROTO_ERROR) has been added.
Which error transmission format (default or extended)
will be used depends on the session negotiation parameters.
@TarantoolBot document
Title: error: extended error transfer format
The error_extended option has been added to connection options.
This option adds a negotiation phase to the connection establishing.
It tries to establish a connection with the possibility of
transmitting errors in an extended format and throws an error
in case of failure.
The extended format also transmit a backtrace and custom type.
The new IPROTO_ERROR representation (using an array(MP_ARRAY)
is useful for the stacked diagnostics task)
0 5
+------++================+================++===================+
| || | || |
| BODY || 0x00: 0x8XXX | 0x01: SYNC || 0x31: ERROR |
|HEADER|| MP_INT: MP_INT | MP_INT: MP_INT || MP_INT: MP_ARRAY |
| SIZE || | || |
+------++================+================++===================+
MP_INT MP_MAP MP_MAP
Every error is a map with keys like ERROR_MSG, ERROR_BT ...
Example:
local connection = netbox.connect(host, port, {error_extended = true})
Needed for #4398
---
src/box/iproto.cc | 20 ++++++++++---
src/box/iproto_constants.h | 14 +++++++++
src/box/lua/error.cc | 21 +++++++++++++
src/box/lua/net_box.lua | 36 ++++++++++++++++++++++-
src/box/session.h | 4 ++-
src/box/xrow.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
src/box/xrow.h | 10 ++++++-
7 files changed, 171 insertions(+), 7 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 2df41ba..bb494f4 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1506,9 +1506,15 @@ tx_accept_msg(struct cmsg *m)
static void
tx_reply_error(struct iproto_msg *msg)
{
+ struct session *ses = msg->connection->session;
struct obuf *out = msg->connection->tx.p_obuf;
- iproto_reply_error(out, diag_last_error(&fiber()->diag),
- msg->header.sync, ::schema_version);
+ if (ses->neg_param.err_format_ver == ERR_FORMAT_EX) {
+ iproto_reply_error_ex(out, diag_last_error(&fiber()->diag),
+ msg->header.sync, ::schema_version);
+ } else {
+ iproto_reply_error(out, diag_last_error(&fiber()->diag),
+ msg->header.sync, ::schema_version);
+ }
iproto_wpos_create(&msg->wpos, out);
}
@@ -1520,9 +1526,15 @@ static void
tx_reply_iproto_error(struct cmsg *m)
{
struct iproto_msg *msg = tx_accept_msg(m);
+ struct session *ses = msg->connection->session;
struct obuf *out = msg->connection->tx.p_obuf;
- iproto_reply_error(out, diag_last_error(&msg->diag),
- msg->header.sync, ::schema_version);
+ if (ses->neg_param.err_format_ver == ERR_FORMAT_EX) {
+ iproto_reply_error_ex(out, diag_last_error(&msg->diag),
+ msg->header.sync, ::schema_version);
+ } else {
+ iproto_reply_error(out, diag_last_error(&msg->diag),
+ msg->header.sync, ::schema_version);
+ }
iproto_wpos_create(&msg->wpos, out);
}
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 1851712..058e07f 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -471,6 +471,20 @@ vy_row_index_key_name(enum vy_row_index_key key)
}
/**
+ * Error details keys
+ */
+enum error_details_key {
+ /** Reason */
+ ERROR_DET_REASON,
+ /** Error code */
+ ERROR_DET_CODE,
+ /** Backtrace */
+ ERROR_DET_BT,
+ /** Custom type */
+ ERROR_DET_CT
+};
+
+/**
* Negotiation protocol's keys
*/
enum neg_key {
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 708d338..4a66335 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -189,6 +189,23 @@ luaT_error_custom_type(lua_State *L)
}
static int
+luaT_error_set_lua_bt(lua_State *L)
+{
+ if (lua_gettop(L) < 2)
+ return luaL_error(L, "Usage: box.error.set_lua_bt(error, bt)");
+
+ struct error *e = luaL_checkerror(L, 1);
+
+ if (lua_type(L, 2) == LUA_TSTRING) {
+ error_set_lua_bt(e, lua_tostring(L, 2));
+ } else {
+ return luaL_error(L, "Usage: box.error.set_lua_bt(error, bt)");
+ }
+
+ return 0;
+}
+
+static int
luaT_error_clear(lua_State *L)
{
if (lua_gettop(L) >= 1)
@@ -316,6 +333,10 @@ box_lua_error_init(struct lua_State *L) {
lua_pushcfunction(L, luaT_error_custom_type);
lua_setfield(L, -2, "custom_type");
}
+ {
+ lua_pushcfunction(L, luaT_error_set_lua_bt);
+ lua_setfield(L, -2, "set_lua_bt");
+ }
lua_setfield(L, -2, "__index");
}
lua_setmetatable(L, -2);
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 7fdad64..7fd317c 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -9,6 +9,7 @@ local errno = require('errno')
local urilib = require('uri')
local internal = require('net.box.lib')
local trigger = require('internal.trigger')
+local debug = require('debug')
local band = bit.band
local max = math.max
@@ -64,6 +65,14 @@ local neg_keys = {
ERROR_FORMAT_VERSION = 0
}
+-- error details
+local error_details = {
+ REASON = 0,
+ CODE = 1,
+ BACKTRACE = 2,
+ CUSTOM_TYPE = 3
+}
+
local function decode_nil(raw_data, raw_data_end)
return nil, raw_data_end
end
@@ -291,7 +300,32 @@ local function create_transport(host, port, user, password, callback,
-- @retval nil, error Error occured.
--
function request_index:result()
- if self.errno then
+ local function parse_extended_error(error_ex)
+ local reason = error_ex[error_details.REASON]
+ local code = error_ex[error_details.CODE]
+ local r_bt = error_ex[error_details.BACKTRACE]
+ local custom_type = error_ex[error_details.CUSTOM_TYPE]
+ local err = box.error.new({code = code,
+ reason = reason,
+ custom_type = custom_type})
+ local cur_bt = debug.traceback()
+ local result_bt = string.format("Remote(%s:%s):"
+ .."\n%s\nEnd Remote\n%s",
+ host, tostring(port),
+ r_bt, cur_bt)
+ box.error.set_lua_bt(err, result_bt)
+ return err
+ end
+
+ if self.errno and type(self.response) == 'table' then
+ local response = self.response[1]
+ if not response then
+ return nil, box.error.new(box.error.PROC_LUA,
+ 'Can\'t parse a remote error')
+ end
+ local err = parse_extended_error(response)
+ return nil, err
+ elseif self.errno then
return nil, box.error.new({code = self.errno,
reason = self.response})
elseif not self.id then
diff --git a/src/box/session.h b/src/box/session.h
index 3b4dfb0..c775fd0 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -87,7 +87,9 @@ enum error_formats {
/** Default(old) format */
ERR_FORMAT_DEF,
/** Extended format */
- ERR_FORMAT_EX
+ ERR_FORMAT_EX,
+ /** The max version of error format */
+ ERR_FORMAT_UNK
};
/**
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 90dffea..4cda99a 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -385,6 +385,10 @@ static const struct iproto_body_bin iproto_error_bin = {
0x81, IPROTO_ERROR, 0xdb, 0
};
+static const struct iproto_body_bin iproto_error_bin_ex = {
+ 0x81, IPROTO_ERROR, 0xdd, 0
+};
+
/** Return a 4-byte numeric error code, with status flags. */
static inline uint32_t
iproto_encode_error(uint32_t error)
@@ -534,6 +538,70 @@ iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
obuf_dup(out, e->errmsg, msg_len) != msg_len ? -1 : 0;
}
+int
+iproto_reply_error_ex(struct obuf *out, const struct error *e,
+ uint64_t sync, uint32_t schema_version)
+{
+ uint32_t errcode = box_error_code(e);
+ const char *err_type = box_error_type(e);
+ const char *custom_type = NULL;
+
+ struct iproto_body_bin body = iproto_error_bin_ex;
+
+ uint32_t buf_size = IPROTO_HEADER_LEN + sizeof(body);
+ /* Reason and code are the necessary fields */
+ uint32_t details_num = 2;
+
+ buf_size += mp_sizeof_uint(ERROR_DET_CODE);
+ buf_size += mp_sizeof_uint(errcode);
+ buf_size += mp_sizeof_uint(ERROR_DET_REASON);
+ buf_size += mp_sizeof_str(strlen(e->errmsg));
+ if (strcmp(err_type, "CustomError") == 0) {
+ ++details_num;
+ buf_size += mp_sizeof_uint(ERROR_DET_CT);
+ custom_type = box_custom_error_type(e);
+ buf_size += mp_sizeof_str(strlen(custom_type));
+ }
+ if (e->lua_bt) {
+ ++details_num;
+ buf_size += mp_sizeof_uint(ERROR_DET_BT);
+ buf_size += mp_sizeof_str(strlen(e->lua_bt));
+ }
+ buf_size += mp_sizeof_map(details_num);
+
+ char *buf = (char *)obuf_alloc(out, buf_size);
+ if (buf == NULL) {
+ diag_set(OutOfMemory, buf_size, "obuf_alloc", "buf");
+ return -1;
+ }
+
+ char *data = buf;
+ iproto_header_encode(data, iproto_encode_error(errcode), sync,
+ schema_version, buf_size - IPROTO_HEADER_LEN);
+ body.v_data_len = mp_bswap_u32(1);
+ data += IPROTO_HEADER_LEN;
+
+ memcpy(data, &body, sizeof(body));
+ data += sizeof(body);
+
+ data = mp_encode_map(data, details_num);
+ data = mp_encode_uint(data, ERROR_DET_CODE);
+ data = mp_encode_uint(data, errcode);
+ data = mp_encode_uint(data, ERROR_DET_REASON);
+ data = mp_encode_str(data, e->errmsg, strlen(e->errmsg));
+ if (custom_type) {
+ data = mp_encode_uint(data, ERROR_DET_CT);
+ data = mp_encode_str(data, custom_type, strlen(custom_type));
+ }
+ if(e->lua_bt) {
+ data = mp_encode_uint(data, ERROR_DET_BT);
+ data = mp_encode_str(data, e->lua_bt, strlen(e->lua_bt));
+ }
+ assert(data == buf + buf_size);
+
+ return 0;
+}
+
void
iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
uint64_t sync)
@@ -1591,6 +1659,11 @@ error:
if (mp_typeof(*data) != MP_UINT)
goto error;
request->err_format_ver = mp_decode_uint(&data);
+ if (request->err_format_ver >= ERR_FORMAT_UNK) {
+ diag_set(ClientError, ER_ILLEGAL_PARAMS,
+ "unknown version of error format");
+ return -1;
+ }
break;
default:
/* unknown key */
diff --git a/src/box/xrow.h b/src/box/xrow.h
index bbd0496..be06003 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -571,7 +571,7 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot,
uint64_t sync, uint32_t schema_version);
/**
- * Write an error packet int output buffer. Doesn't throw if out
+ * Write an error packet to output buffer. Doesn't throw if out
* of memory
*/
int
@@ -579,6 +579,14 @@ iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
uint32_t schema_version);
/**
+ * Write an extended error packet to output buffer. Doesn't throw if out
+ * of memory
+ */
+int
+iproto_reply_error_ex(struct obuf *out, const struct error *e, uint64_t sync,
+ uint32_t schema_version);
+
+/**
* Write a negotiation reply packet to output buffer.
* @param out Buffer to write to.
* @param neg_param Current negotiation parameters.
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Tarantool-patches] [PATCH 5/6] error: Add test for extended error
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
` (3 preceding siblings ...)
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 4/6] error: Add extended error transfer format Leonid Vasiliev
@ 2020-03-24 12:46 ` Leonid Vasiliev
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 6/6] error: Transmit an error through IPROTO_OK as object Leonid Vasiliev
` (2 subsequent siblings)
7 siblings, 0 replies; 30+ messages in thread
From: Leonid Vasiliev @ 2020-03-24 12:46 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
Needed for #4398
---
test/box-tap/extended_error.test.lua | 168 +++++++++++++++++++++++++++++++++++
1 file changed, 168 insertions(+)
create mode 100755 test/box-tap/extended_error.test.lua
diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
new file mode 100755
index 0000000..58a4984
--- /dev/null
+++ b/test/box-tap/extended_error.test.lua
@@ -0,0 +1,168 @@
+#! /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(2)
+
+
+function error_func()
+ box.error(box.error.PROC_LUA, "An old good error")
+end
+
+function custom_error_func()
+ box.error(box.error.CUSTOM_ERROR, "My Custom Type", "A modern custom error")
+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.type ~= v then
+ return false
+ end
+ elseif k == 'custom_type' then
+ if err.custom_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 == 'bt' then
+ if not string.match(err.bt, v) then
+ return false
+ end
+ elseif k == 'is_custom' then
+ if err:is_custom() ~= 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')
+
+ local connection = netbox.connect(host, port)
+ local _, err = pcall(connection.call, connection, 'error_func')
+
+ local backtrace_pattern =
+ "^stack traceback:\n" ..
+ "%s+%[C%]: in function 'new'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function 'perform_request'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function '_request'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function <builtin/box/net_box.lua:%d+>\n" ..
+ "%s+%[C%]: in function 'pcall'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in function 'test_old_transmission'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in main chunk$"
+
+ local check_list = {
+ type = 'ClientError',
+ message = 'An old good error',
+ trace = '^File builtin/box/net_box.lua\nLine %d+$',
+ bt = backtrace_pattern,
+ is_custom = false
+ }
+
+ local check_result = check_error(err, check_list)
+ test:ok(check_result, 'Check the old transmission type')
+ connection:close()
+end
+
+local function test_extended_transmission(host, port)
+ grant_func('guest', 'custom_error_func')
+
+ local connection = netbox.connect(host, port, {error_extended = true})
+ local _, err = pcall(connection.call, connection, 'custom_error_func')
+
+ local backtrace_pattern =
+ "^Remote%(.+:%d+%):\n" ..
+ "stack traceback:\n" ..
+ "%s+%[C%]: in function 'error'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in function <.*box%-tap/extended_error.test.lua:%d+>\n" ..
+ "%s+%[C%]: at 0x%w+\n" ..
+ "End Remote\n" ..
+ "stack traceback:\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function 'parse_extended_error'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function 'perform_request'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function '_request'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function <builtin/box/net_box.lua:%d+>\n" ..
+ "%s+%[C%]: in function 'pcall'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in function 'test_extended_transmission'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in main chunk$"
+
+ local check_list = {
+ type = 'CustomError',
+ custom_type = 'My Custom Type',
+ message = 'User custom error: A modern custom error',
+ trace = '^File builtin/box/net_box.lua\nLine %d+$',
+ bt = backtrace_pattern,
+ is_custom = true
+ }
+
+ local check_result = check_error(err, check_list)
+ test:ok(check_result, 'Check the extended transmission type')
+ connection:close()
+end
+
+
+box.cfg{
+ listen = os.getenv('LISTEN')
+}
+local tarantool_ver = string.match(box.info.version, "%d%.%d%.%d")
+local host= uri.parse(box.cfg.listen).host or 'localhost'
+local port = uri.parse(box.cfg.listen).service
+
+if version_at_least(box.info.version, 2, 4, 1) then
+ test_extended_transmission(host, port)
+else
+ test:ok(true, 'Current version of tarantool(' .. tarantool_ver .. ')' ..
+ ' don\'t support extended transmission')
+end
+test_old_transmission(host, port)
+
+os.exit(test:check() and 0 or 1)
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Tarantool-patches] [PATCH 6/6] error: Transmit an error through IPROTO_OK as object
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
` (4 preceding siblings ...)
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 5/6] error: Add test for extended error Leonid Vasiliev
@ 2020-03-24 12:46 ` Leonid Vasiliev
2020-03-27 23:11 ` [Tarantool-patches] [PATCH 0/6] Extending error functionality lvasiliev
2020-04-01 15:35 ` Alexander Turenko
7 siblings, 0 replies; 30+ messages in thread
From: Leonid Vasiliev @ 2020-03-24 12:46 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
An ability to transfer an error through IPROTO_OK as object has been added.
Accordinally to https://github.com/tarantool/tarantool/issues/4398
ability to transfer an error through net.box (an IPROTO_OK case)
has been added. The error will be transmit through net.box as object
if the extended transfer error format is used. In this case
for serialization will be used a __serialize_ex method.
@TarantoolBot document
Title: error: transmit an error object throw IPROTO_OK
The error will be transmitted through IPROTO_OK as an object
if the extended transmission error format is used
(before it was transmitted as a string).
---
src/box/execute.c | 1 +
src/box/lua/call.c | 29 ++++++++++++---------
src/box/lua/error.cc | 13 ++++++++++
src/box/lua/execute.c | 2 +-
src/box/lua/net_box.lua | 10 ++++++++
src/box/lua/tuple.c | 12 ++++-----
src/box/session.cc | 4 +++
src/box/session.h | 17 +++---------
src/box/sql/func.c | 4 ++-
src/lib/core/mp_extension_types.h | 4 +--
src/lib/core/port.h | 4 +--
src/lua/error.c | 2 --
src/lua/error.h | 3 ++-
src/lua/error.lua | 22 +++++++++++++++-
src/lua/msgpack.c | 26 ++++++++++---------
src/lua/msgpack.h | 8 +++---
src/lua/utils.c | 28 +++++++++++++++-----
src/lua/utils.h | 27 +++++++++++++++++--
test/box-tap/extended_error.test.lua | 50 +++++++++++++++++++++++++++++++++---
19 files changed, 199 insertions(+), 67 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 24f8529..e48c694 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -49,6 +49,7 @@
#include "box/sql_stmt_cache.h"
#include "session.h"
#include "rmean.h"
+#include "lua/utils.h"
const char *sql_info_key_strs[] = {
"row_count",
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index f1bbde7..8e2d5d2 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.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,7 @@ execute_lua_eval(lua_State *L)
struct encode_lua_ctx {
struct port_lua *port;
struct mpstream *stream;
+ struct luaL_serializer_ctx *serializer_ctx;
};
static int
@@ -393,8 +395,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_ctx,
+ ctx->stream, i);
+ }
ctx->port->size = size;
mpstream_flush(ctx->stream);
return 0;
@@ -429,6 +433,7 @@ port_lua_do_dump(struct port *base, struct mpstream *stream,
struct encode_lua_ctx ctx;
ctx.port = port;
ctx.stream = stream;
+ ctx.serializer_ctx = ¤t_session()->serializer_ctx;
struct lua_State *L = tarantool_L;
int top = lua_gettop(L);
if (lua_cpcall(L, handler, &ctx) != 0) {
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 4a66335..efe9e8e 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -189,6 +189,15 @@ luaT_error_custom_type(lua_State *L)
}
static int
+luaT_error_code(lua_State *L)
+{
+ struct error *e = luaL_checkerror(L, -1);
+ const uint32_t code = box_error_code(e);
+ lua_pushinteger(L, code);
+ return 1;
+}
+
+static int
luaT_error_set_lua_bt(lua_State *L)
{
if (lua_gettop(L) < 2)
@@ -337,6 +346,10 @@ box_lua_error_init(struct lua_State *L) {
lua_pushcfunction(L, luaT_error_set_lua_bt);
lua_setfield(L, -2, "set_lua_bt");
}
+ {
+ lua_pushcfunction(L, luaT_error_code);
+ lua_setfield(L, -2, "error_code");
+ }
lua_setfield(L, -2, "__index");
}
lua_setmetatable(L, -2);
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/net_box.lua b/src/box/lua/net_box.lua
index 7fd317c..281b465 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -329,6 +329,15 @@ local function create_transport(host, port, user, password, callback,
return nil, box.error.new({code = self.errno,
reason = self.response})
elseif not self.id then
+ if type(self.response) == 'table' then
+ for k, v in pairs(self.response) do
+ if type(v) == 'table' and v['error_magic']
+ and v['error_magic'] == 13371338 then
+ local err = parse_extended_error(v)
+ self.response[k] = err
+ end
+ end
+ end
return self.response
elseif not worker_fiber then
return nil, box.error.new(E_NO_CONNECTION)
@@ -632,6 +641,7 @@ local function create_transport(host, port, user, password, callback,
ffi.copy(wpos, body_rpos, body_len)
body_len = tonumber(body_len)
if status == IPROTO_OK_KEY then
+ -- tuta interesno
request.response = body_len
requests[id] = nil
request.id = nil
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 1e3c3d6..18efd1e 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 c0224ca..6e13fd2 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -148,6 +148,9 @@ session_create(enum session_type type)
/* Set default negotiation parameters */
session->neg_param.err_format_ver = ERR_FORMAT_DEF;
+ /* Set default Lua serializer context */
+ session->serializer_ctx.err_format_ver = ERR_FORMAT_DEF;
+
/* For on_connect triggers. */
credentials_create(&session->credentials, guest_user);
struct mh_i64ptr_node_t node;
@@ -383,5 +386,6 @@ session_update_neg_parameters(struct session *session,
const struct negotiation_params *params)
{
session->neg_param.err_format_ver = params->err_format_ver;
+ session->serializer_ctx.err_format_ver = params->err_format_ver;
return 0;
}
diff --git a/src/box/session.h b/src/box/session.h
index c775fd0..91b6850 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -36,6 +36,7 @@
#include "fiber.h"
#include "user.h"
#include "authentication.h"
+#include "lua/utils.h"
#if defined(__cplusplus)
extern "C" {
@@ -81,20 +82,8 @@ union session_meta {
};
/**
- * An error transmission formats
- */
-enum error_formats {
- /** Default(old) format */
- ERR_FORMAT_DEF,
- /** Extended format */
- ERR_FORMAT_EX,
- /** The max version of error format */
- ERR_FORMAT_UNK
-};
-
-/**
* Parameters which may be changed at negotiation phase of session
-*/
+ */
struct negotiation_params {
/** Version of a format for an error transmission */
uint8_t err_format_ver;
@@ -132,6 +121,8 @@ struct session {
struct trigger fiber_on_stop;
/** Negotiation parameters */
struct negotiation_params neg_param;
+ /** Session Lua serializer context */
+ struct luaL_serializer_ctx serializer_ctx;
};
struct session_vtab {
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 6e724c8..bbc1f6f 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/lib/core/mp_extension_types.h b/src/lib/core/mp_extension_types.h
index bc9873f..0c0fae0 100644
--- a/src/lib/core/mp_extension_types.h
+++ b/src/lib/core/mp_extension_types.h
@@ -40,8 +40,8 @@
* You may assign values in range [0, 127]
*/
enum mp_extension_type {
- MP_UNKNOWN_EXTENSION = 0,
- MP_DECIMAL = 1,
+ MP_UNKNOWN_EXTENSION = 0,
+ MP_DECIMAL = 1,
};
#endif
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index bfdfa46..2aa9338 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -136,9 +136,9 @@ port_dump_msgpack(struct port *port, struct obuf *out)
}
static inline int
-port_dump_msgpack_16(struct port *port, struct obuf *out)
+port_dump_msgpack_16(struct port *port, struct obuf *buf)
{
- return port->vtab->dump_msgpack_16(port, out);
+ return port->vtab->dump_msgpack_16(port, buf);
}
static inline void
diff --git a/src/lua/error.c b/src/lua/error.c
index 3a12e20..c72f835 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -36,8 +36,6 @@
#include <string.h>
-static int CTID_CONST_STRUCT_ERROR_REF = 0;
-
/*
* Memory for the traceback string is obtained with malloc,
* and can be freed with free.
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/error.lua b/src/lua/error.lua
index 26abec8..da6b3cc 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -33,6 +33,14 @@ int
exception_get_int(struct error *e, const struct method_info *method);
]]
+-- error details
+local error_details = {
+ REASON = 0,
+ CODE = 1,
+ BACKTRACE = 2,
+ CUSTOM_TYPE = 3
+}
+
local REFLECTION_CACHE = {}
local function reflection_enumerate(err)
@@ -152,6 +160,17 @@ local function error_serialize(err)
return error_message(err)
end
+local function error_serialize_ex(err)
+ local result = {
+ ["error_magic"] = 13371338,
+ [error_details.CODE] = box.error.error_code(err),
+ [error_details.CUSTOM_TYPE] = error_custom_type(err),
+ [error_details.REASON] = error_message(err),
+ [error_details.BACKTRACE] = error_backtrace(err)
+ }
+ return result
+end
+
local function error_is_custom(err)
return ffi.string(err._type.name) == 'CustomError'
end
@@ -161,7 +180,8 @@ local error_methods = {
["raise"] = error_raise,
["match"] = error_match, -- Tarantool 1.6 backward compatibility
["is_custom"] = error_is_custom,
- ["__serialize"] = error_serialize
+ ["__serialize"] = error_serialize,
+ ["__serialize_ex"] = error_serialize_ex
}
local function error_index(err, key)
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index edbc15b..e1f0e8c 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -108,8 +108,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 luaL_serializer_ctx *ctx, struct mpstream *stream,
+ struct luaL_field *field, int level)
{
int top = lua_gettop(L);
enum mp_type type;
@@ -154,13 +154,13 @@ restart: /* used by MP_EXT */
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, ctx, lua_gettop(L), field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, ctx, 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, ctx, lua_gettop(L), field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, ctx, stream, field, level + 1);
lua_pop(L, 1); /* pop value */
}
assert(lua_gettop(L) == top);
@@ -179,9 +179,9 @@ restart: /* used by MP_EXT */
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, ctx, top + 1, field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, ctx, stream, field, level + 1);
lua_pop(L, 1);
}
assert(lua_gettop(L) == top);
@@ -209,7 +209,8 @@ restart: /* used by MP_EXT */
enum mp_type
luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, int index)
+ struct luaL_serializer_ctx *ctx, struct mpstream *stream,
+ int index)
{
int top = lua_gettop(L);
if (index < 0)
@@ -221,9 +222,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, ctx, 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, ctx, stream, &field, 0);
if (!on_top) {
lua_remove(L, top + 1); /* remove a value copy */
@@ -232,6 +233,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)
@@ -354,7 +356,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..e5c7a94 100644
--- a/src/lua/msgpack.h
+++ b/src/lua/msgpack.h
@@ -43,6 +43,7 @@ extern "C" {
struct luaL_serializer;
struct mpstream;
+struct luaL_serializer_ctx;
/**
* 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 luaL_serializer_ctx *ctx, 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 luaL_serializer_ctx *ctx, 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 de14c77..9a2b0b9 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -46,6 +46,7 @@ static uint32_t CTID_STRUCT_IBUF_PTR;
static uint32_t CTID_CHAR_PTR;
static uint32_t CTID_CONST_CHAR_PTR;
uint32_t CTID_DECIMAL;
+uint32_t CTID_CONST_STRUCT_ERROR_REF;
void *
@@ -436,16 +437,21 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
int top = lua_gettop(L);
lua_pushcfunction(L, lua_gettable_wrapper);
lua_pushvalue(L, idx);
- lua_pushliteral(L, LUAL_SERIALIZE);
+ const char *serialize = LUAL_SERIALIZE;
+ if (field->use_serialize_ex)
+ serialize = LUAL_SERIALIZE_EX;
+
+ lua_pushstring(L, serialize);
+
if (lua_pcall(L, 2, 1, 0) == 0 && !lua_isnil(L, -1)) {
if (!lua_isfunction(L, -1))
- luaL_error(L, "invalid " LUAL_SERIALIZE " value");
+ luaL_error(L, "invalid %s value", serialize);
/* copy object itself */
lua_pushvalue(L, idx);
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 */
@@ -508,7 +514,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;
}
@@ -634,12 +640,13 @@ 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 luaL_serializer_ctx *ctx, int index,
struct luaL_field *field)
{
if (index < 0)
@@ -649,6 +656,8 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
double intpart;
size_t size;
+ field->use_serialize_ex = false;
+
#define CHECK_NUMBER(x) ({ \
if (!isfinite(x) && !cfg->encode_invalid_numbers) { \
if (!cfg->encode_invalid_as_nil) { \
@@ -746,6 +755,11 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
if (cd->ctypeid == CTID_DECIMAL) {
field->ext_type = MP_DECIMAL;
field->decval = (decimal_t *) cdata;
+ } else if (cd->ctypeid == CTID_CONST_STRUCT_ERROR_REF
+ && ctx
+ && ctx->err_format_ver == ERR_FORMAT_EX) {
+ field->ext_type = MP_UNKNOWN_EXTENSION;
+ field->use_serialize_ex = true;
} else {
field->ext_type = MP_UNKNOWN_EXTENSION;
}
@@ -781,6 +795,7 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
default:
field->type = MP_EXT;
field->ext_type = MP_UNKNOWN_EXTENSION;
+ field->use_serialize_ex = false;
}
#undef CHECK_NUMBER
return 0;
@@ -792,6 +807,7 @@ luaL_convertfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
{
if (idx < 0)
idx = lua_gettop(L) + idx + 1;
+
assert(field->type == MP_EXT && field->ext_type == MP_UNKNOWN_EXTENSION); /* must be called after tofield() */
if (cfg->encode_load_metatables) {
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 0b36727..9e02003 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -266,12 +266,32 @@ struct luaL_serializer {
struct rlist on_update;
};
+/**
+ * An error serialization formats
+ */
+enum error_formats {
+ /** Default(old) format */
+ ERR_FORMAT_DEF,
+ /** Extended format */
+ ERR_FORMAT_EX,
+ /** The max version of error format */
+ ERR_FORMAT_UNK
+};
+
+/**
+ * A serializer context (additional settings for a serializer)
+ */
+struct luaL_serializer_ctx {
+ uint8_t err_format_ver;
+};
+
extern int luaL_nil_ref;
extern int luaL_map_metatable_ref;
extern int luaL_array_metatable_ref;
#define LUAL_SERIALIZER "serializer"
#define LUAL_SERIALIZE "__serialize"
+#define LUAL_SERIALIZE_EX "__serialize_ex"
struct luaL_serializer *
luaL_newserializer(struct lua_State *L, const char *modname, const luaL_Reg *reg);
@@ -325,6 +345,7 @@ struct luaL_field {
/* subtypes of MP_EXT */
enum mp_extension_type ext_type;
bool compact; /* a flag used by YAML serializer */
+ bool use_serialize_ex;
};
/**
@@ -361,6 +382,7 @@ struct luaL_field {
*
* @param L stack
* @param cfg configuration
+ * @param serializer_ctx the Lua serializer context
* @param index stack index
* @param field conversion result
*
@@ -368,7 +390,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 luaL_serializer_ctx *ctx, int index,
struct luaL_field *field);
/**
@@ -407,7 +430,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/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
index 58a4984..b38a7ab 100755
--- a/test/box-tap/extended_error.test.lua
+++ b/test/box-tap/extended_error.test.lua
@@ -6,7 +6,7 @@ local tap = require('tap')
local uri = require('uri')
local test = tap.test('check an extended error')
-test:plan(2)
+test:plan(4)
function error_func()
@@ -17,6 +17,12 @@ function custom_error_func()
box.error(box.error.CUSTOM_ERROR, "My Custom Type", "A modern custom error")
end
+function custom_error_func_2()
+ local err = box.error.new(box.error.CUSTOM_ERROR,
+ "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)")
@@ -86,9 +92,11 @@ 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 backtrace_pattern =
"^stack traceback:\n" ..
@@ -109,15 +117,22 @@ local function test_old_transmission(host, port)
}
local check_result = check_error(err, check_list)
- test:ok(check_result, 'Check the old transmission type')
+ local check_result_2 = type(err_2) == 'string'
+ and err_2 == 'User custom error: 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')
local connection = netbox.connect(host, port, {error_extended = true})
local _, err = pcall(connection.call, connection, 'custom_error_func')
+ local err_2 = connection:call('custom_error_func_2')
local backtrace_pattern =
"^Remote%(.+:%d+%):\n" ..
@@ -135,6 +150,21 @@ local function test_extended_transmission(host, port)
".*box%-tap/extended_error.test.lua:%d+: in function 'test_extended_transmission'\n" ..
".*box%-tap/extended_error.test.lua:%d+: in main chunk$"
+ local backtrace_pattern_2 =
+ "^Remote%(.+:%d+%):\n" ..
+ "stack traceback:\n" ..
+ "%s+%[C%]: in function 'new'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in function <.*box%-tap/extended_error.test.lua:%d+>\n" ..
+ "%s+%[C%]: at 0x%w+\n" ..
+ "End Remote\n" ..
+ "stack traceback:\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function 'parse_extended_error'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function 'perform_request'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function '_request'\n" ..
+ "%s+builtin/box/net_box.lua:%d+: in function 'call'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in function 'test_extended_transmission'\n" ..
+ ".*box%-tap/extended_error.test.lua:%d+: in main chunk$"
+
local check_list = {
type = 'CustomError',
custom_type = 'My Custom Type',
@@ -144,8 +174,20 @@ local function test_extended_transmission(host, port)
is_custom = true
}
+ local check_list_2 = {
+ type = 'CustomError',
+ custom_type = 'My Custom Type',
+ message = 'User custom error: A modern custom error',
+ trace = '^File builtin/box/net_box.lua\nLine %d+$',
+ bt = backtrace_pattern_2,
+ is_custom = true
+ }
+
local check_result = check_error(err, check_list)
- test:ok(check_result, 'Check the extended transmission type')
+ 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
@@ -162,6 +204,8 @@ if version_at_least(box.info.version, 2, 4, 1) then
else
test:ok(true, 'Current version of tarantool(' .. tarantool_ver .. ')' ..
' don\'t support extended transmission')
+ test:ok(true, 'Current version of tarantool(' .. tarantool_ver .. ')' ..
+ ' don\'t support extended transmission')
end
test_old_transmission(host, port)
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase Leonid Vasiliev
@ 2020-03-24 20:02 ` Konstantin Osipov
2020-03-25 7:35 ` lvasiliev
0 siblings, 1 reply; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-24 20:02 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches
* Leonid Vasiliev <lvasiliev@tarantool.org> [20/03/24 16:02]:
> The negotiation phase has been added to IPROTO
>
> For possibility to have a custom parameters of session the negotiation
> phase has been added. This is necessary to enable the transmission of
> an error in different formats(depending on the choice of the client).
>
> @TarantoolBot document
> Title: IPROTO: The negatiation phase
> For backward compatibility of the data transmission format,
> the negotiation phase has been added to IPROTO.
> A new key (IPROTO_NEGOTIATION) has been added to IPROTO command codes.
> NEGOTIATION BODY: CODE = 0x0E
> +==========================+
> | |
> | NEGOTIATION PARAMETERS |
> | |
> +==========================+
> MP_MAP
> Session negotiation parameters are a map with keys like ERROR_FORMAT_VERSION ...
> The response is a map with all stated negotiation parameters.
> So, for work with the new format of errors, it is necessary to perform the negotiation phase,
> otherwise errors will be transmitted in the old format (by default).
Why not make it a key in IPROTO_AUTH, and require a separate
round-trip?
Why have it at all and not look at server version, which is part
of the greeting already?
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-24 20:02 ` Konstantin Osipov
@ 2020-03-25 7:35 ` lvasiliev
2020-03-25 8:42 ` Konstantin Osipov
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: lvasiliev @ 2020-03-25 7:35 UTC (permalink / raw)
To: Konstantin Osipov, alexander.turenko, tarantool-patches
On 24.03.2020 23:02, Konstantin Osipov wrote:
> * Leonid Vasiliev <lvasiliev@tarantool.org> [20/03/24 16:02]:
>> The negotiation phase has been added to IPROTO
>>
>> For possibility to have a custom parameters of session the negotiation
>> phase has been added. This is necessary to enable the transmission of
>> an error in different formats(depending on the choice of the client).
>>
>> @TarantoolBot document
>> Title: IPROTO: The negatiation phase
>> For backward compatibility of the data transmission format,
>> the negotiation phase has been added to IPROTO.
>> A new key (IPROTO_NEGOTIATION) has been added to IPROTO command codes.
>> NEGOTIATION BODY: CODE = 0x0E
>> +==========================+
>> | |
>> | NEGOTIATION PARAMETERS |
>> | |
>> +==========================+
>> MP_MAP
>> Session negotiation parameters are a map with keys like ERROR_FORMAT_VERSION ...
>> The response is a map with all stated negotiation parameters.
>> So, for work with the new format of errors, it is necessary to perform the negotiation phase,
>> otherwise errors will be transmitted in the old format (by default).
>
> Why not make it a key in IPROTO_AUTH, and require a separate
> round-trip?
Hi. Because it's not a part of AAA.
>
> Why have it at all and not look at server version, which is part
> of the greeting already?
>
The cause is backward compatibility.
For example: a client application may expect an error as a string
(IPROTO_OK case) and instead of which it will receive an error as an
“object”. A greeting is sent only from the server side to the client,
but the server must know what format should be used to send errors (what
format does the client expect).
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-25 7:35 ` lvasiliev
@ 2020-03-25 8:42 ` Konstantin Osipov
2020-03-25 10:56 ` Eugene Leonovich
2020-03-26 11:18 ` lvasiliev
2020-03-26 21:13 ` Alexander Turenko
2020-03-26 23:35 ` Alexander Turenko
2 siblings, 2 replies; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-25 8:42 UTC (permalink / raw)
To: lvasiliev; +Cc: tarantool-patches
* lvasiliev <lvasiliev@tarantool.org> [20/03/25 10:36]:
> > Why not make it a key in IPROTO_AUTH, and require a separate
> > round-trip?
> Hi. Because it's not a part of AAA.
1. Auth packet is a session control packet. Authentication
is only one thing you may want to do with the session, you may
want to simply switch between already authenticated users.
2. Adding an extra package adds an extra round-trip and slows down each
handshake by at least 1 millisecond.
3. Auth package is synchronous by default. Any other package is
not, it's executed in async mode. Meaning the semantics of the
new package is broken, it is unclear what responses it affects
- those that happen to be not sent yet when this package is
handled, but not necessarily those that started processing
after this package arrived.
> > Why have it at all and not look at server version, which is part
> > of the greeting already?
> >
> The cause is backward compatibility.
> For example: a client application may expect an error as a string (IPROTO_OK
> case) and instead of which it will receive an error as an “object”. A
> greeting is sent only from the server side to the client, but the server
> must know what format should be used to send errors (what format does the
> client expect).
A much simpler way to do it is to have a server switch to enable
new features.
It is less flexible, of course, because you can't have old and new
clients, but do you really want to have old and new clients?
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-25 8:42 ` Konstantin Osipov
@ 2020-03-25 10:56 ` Eugene Leonovich
2020-03-25 11:13 ` Konstantin Osipov
2020-03-26 11:37 ` lvasiliev
2020-03-26 11:18 ` lvasiliev
1 sibling, 2 replies; 30+ messages in thread
From: Eugene Leonovich @ 2020-03-25 10:56 UTC (permalink / raw)
To: Konstantin Osipov, lvasiliev, alexander.turenko, tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]
> A much simpler way to do it is to have a server switch to enable
> new features.
> It is less flexible, of course, because you can't have old and new
> clients, but do you really want to have old and new clients?
I do agree with Kostya, I think it's a good compromise. In my humble
opinion, the extended error feature in its current state is not worth the
complexity and overhead it adds. Instead, why not introduce a Tarantool
setting to choose whether you want to deal with a legacy or extended error
response type (and it's very unlikely that someone will need to have 2
types at the same time). By default this setting will be set to use the
legacy mode, then after N minor 2.x releases, it can be changed to use the
new error type, the setting itself will be marked as deprecated and removed
in the next major release. From a connector's point of view, it will also
be easy to check if there are any additional fields in the response body,
which would mean that the connector has received a "new" error.
--
Thank you and best regards,
Eugene Leonovich
[-- Attachment #2: Type: text/html, Size: 1387 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-25 10:56 ` Eugene Leonovich
@ 2020-03-25 11:13 ` Konstantin Osipov
2020-03-26 11:37 ` lvasiliev
1 sibling, 0 replies; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-25 11:13 UTC (permalink / raw)
To: Eugene Leonovich; +Cc: tarantool-patches
* Eugene Leonovich <gen.work@gmail.com> [20/03/25 14:00]:
> > A much simpler way to do it is to have a server switch to enable
> > new features.
> > It is less flexible, of course, because you can't have old and new
> > clients, but do you really want to have old and new clients?
>
>
> I do agree with Kostya, I think it's a good compromise. In my humble
> opinion, the extended error feature in its current state is not worth the
> complexity and overhead it adds. Instead, why not introduce a Tarantool
> setting to choose whether you want to deal with a legacy or extended error
> response type (and it's very unlikely that someone will need to have 2
> types at the same time). By default this setting will be set to use the
> legacy mode, then after N minor 2.x releases, it can be changed to use the
> new error type, the setting itself will be marked as deprecated and removed
> in the next major release. From a connector's point of view, it will also
> be easy to check if there are any additional fields in the response body,
> which would mean that the connector has received a "new" error.
More community on the patches list! Yay!
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-25 8:42 ` Konstantin Osipov
2020-03-25 10:56 ` Eugene Leonovich
@ 2020-03-26 11:18 ` lvasiliev
2020-03-26 12:16 ` Konstantin Osipov
1 sibling, 1 reply; 30+ messages in thread
From: lvasiliev @ 2020-03-26 11:18 UTC (permalink / raw)
To: Konstantin Osipov, alexander.turenko, tarantool-patches
Hi! Еhanks for the feedback.
On 25.03.2020 11:42, Konstantin Osipov wrote:
> * lvasiliev <lvasiliev@tarantool.org> [20/03/25 10:36]:
>>> Why not make it a key in IPROTO_AUTH, and require a separate
>>> round-trip?
>> Hi. Because it's not a part of AAA.
>
> 1. Auth packet is a session control packet. Authentication
> is only one thing you may want to do with the session, you may
> want to simply switch between already authenticated users.
Now it does't look like a session control packet because auth packet
will be sent to server only if authentication is needed ("Authentication
in Tarantool is optional, if no authentication is performed, session
user is ‘guest’"). But, theoretically, it can be used.
In this case, the answer must be changed from ok/fail to the answer with
payload.
In my opinion the negotiation looks as:
- the client offers options for the session
- the server sends the resulting response with options (which may differ
from the requested)
- the client decides to work with such settings or disconnect
I think that negotiation phase can be used for flexible session setup in
the future (not only for errors)
>
> 2. Adding an extra package adds an extra round-trip and slows down each
> handshake by at least 1 millisecond.
Yes, but it used only if you don't want use default settings of the session.
>
> 3. Auth package is synchronous by default. Any other package is
> not, it's executed in async mode. Meaning the semantics of the
> new package is broken, it is unclear what responses it affects
> - those that happen to be not sent yet when this package is
> handled, but not necessarily those that started processing
> after this package arrived.
I agree that this operation should be synchronous.
As I understand remote:request from the net_box.lua will wait for a
response. Is it incorrectly?
>
>>> Why have it at all and not look at server version, which is part
>>> of the greeting already?
>>>
>> The cause is backward compatibility.
>> For example: a client application may expect an error as a string (IPROTO_OK
>> case) and instead of which it will receive an error as an “object”. A
>> greeting is sent only from the server side to the client, but the server
>> must know what format should be used to send errors (what format does the
>> client expect).
>
> A much simpler way to do it is to have a server switch to enable
> new features.
> It is less flexible, of course, because you can't have old and new
> clients, but do you really want to have old and new clients?
>
It's not about what I want, it's about what is possible and backward
compatibility. I think this is a realistic situation.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-25 10:56 ` Eugene Leonovich
2020-03-25 11:13 ` Konstantin Osipov
@ 2020-03-26 11:37 ` lvasiliev
1 sibling, 0 replies; 30+ messages in thread
From: lvasiliev @ 2020-03-26 11:37 UTC (permalink / raw)
To: Eugene Leonovich, Konstantin Osipov, alexander.turenko,
tarantool-patches
On 25.03.2020 13:56, Eugene Leonovich wrote:
>
> A much simpler way to do it is to have a server switch to enable
> new features.
> It is less flexible, of course, because you can't have old and new
> clients, but do you really want to have old and new clients?
>
>
> I do agree with Kostya, I think it's a good compromise. In my humble
> opinion, the extended error feature in its current state is not worth
> the complexity and overhead it adds. Instead, why not introduce a
> Tarantool setting to choose whether you want to deal with a legacy or
> extended error response type (and it's very unlikely that someone will
> need to have 2 types at the same time). By default this setting will be
> set to use the legacy mode, then after N minor 2.x releases, it can be
> changed to use the new error type, the setting itself will be marked as
> deprecated and removed in the next major release. From a connector's
> point of view, it will also be easy to check if there are any additional
> fields in the response body, which would mean that the connector has
> received a "new" error.
>
> --
> Thank you and best regards,
> Eugene Leonovich
Hi! Thanks for the feedback.
It sounds like a soft break of the backward compatibility.
"From a connector's point of view, it will also be easy to check if
there are any additional fields in the response body, which would mean
that the connector has received a "new" error." - No, the format of a
message has been changed.
About negotiation phase - it's optional and I think it can be used for
flexible session setup in the future (not only for errors).
If you use a default settings of the session negotiation phase is absent
and don't add additional overhad.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-26 11:18 ` lvasiliev
@ 2020-03-26 12:16 ` Konstantin Osipov
2020-03-26 12:54 ` Kirill Yukhin
0 siblings, 1 reply; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-26 12:16 UTC (permalink / raw)
To: lvasiliev; +Cc: tarantool-patches
* lvasiliev <lvasiliev@tarantool.org> [20/03/26 14:18]:
> Now it does't look like a session control packet because auth packet will be
> sent to server only if authentication is needed ("Authentication in
> Tarantool is optional, if no authentication is performed, session user is
> ‘guest’"). But, theoretically, it can be used.
You can authenticate guest to guest, it will work.
> In this case, the answer must be changed from ok/fail to the answer with
> payload.
> In my opinion the negotiation looks as:
> - the client offers options for the session
> - the server sends the resulting response with options (which may differ
> from the requested)
> - the client decides to work with such settings or disconnect
> I think that negotiation phase can be used for flexible session setup in the
> future (not only for errors)
Cost of establishing a connection should be as low as possible.
A sharded tarantool cluster has n^2 of them, and n can be very big
- each cpu core is its own instance. 400 000 connections per
cluster are not uncommon.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-26 12:16 ` Konstantin Osipov
@ 2020-03-26 12:54 ` Kirill Yukhin
2020-03-26 13:19 ` Konstantin Osipov
0 siblings, 1 reply; 30+ messages in thread
From: Kirill Yukhin @ 2020-03-26 12:54 UTC (permalink / raw)
To: Konstantin Osipov, lvasiliev, alexander.turenko, tarantool-patches
Hello,
On 26 мар 15:16, Konstantin Osipov wrote:
> * lvasiliev <lvasiliev@tarantool.org> [20/03/26 14:18]:
> > In this case, the answer must be changed from ok/fail to the answer with
> > payload.
> > In my opinion the negotiation looks as:
> > - the client offers options for the session
> > - the server sends the resulting response with options (which may differ
> > from the requested)
> > - the client decides to work with such settings or disconnect
> > I think that negotiation phase can be used for flexible session setup in the
> > future (not only for errors)
>
> Cost of establishing a connection should be as low as possible.
> A sharded tarantool cluster has n^2 of them, and n can be very big
> - each cpu core is its own instance. 400 000 connections per
> cluster are not uncommon.
I think this is pretty much fable.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-26 12:54 ` Kirill Yukhin
@ 2020-03-26 13:19 ` Konstantin Osipov
2020-03-26 13:31 ` Konstantin Osipov
0 siblings, 1 reply; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-26 13:19 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches
* Kirill Yukhin <kyukhin@tarantool.org> [20/03/26 15:58]:
> > > In this case, the answer must be changed from ok/fail to the answer with
> > > payload.
> > > In my opinion the negotiation looks as:
> > > - the client offers options for the session
> > > - the server sends the resulting response with options (which may differ
> > > from the requested)
> > > - the client decides to work with such settings or disconnect
> > > I think that negotiation phase can be used for flexible session setup in the
> > > future (not only for errors)
> >
> > Cost of establishing a connection should be as low as possible.
> > A sharded tarantool cluster has n^2 of them, and n can be very big
> > - each cpu core is its own instance. 400 000 connections per
> > cluster are not uncommon.
>
> I think this is pretty much fable.
A vshard based cluster with 20 router nodes and 20 storage nodes,
32 core each, will have 20*32*32*16 connections.
A 20-40 node cluster is not big by any means.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-26 13:19 ` Konstantin Osipov
@ 2020-03-26 13:31 ` Konstantin Osipov
0 siblings, 0 replies; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-26 13:31 UTC (permalink / raw)
To: Kirill Yukhin, lvasiliev, alexander.turenko, tarantool-patches
* Konstantin Osipov <kostja.osipov@gmail.com> [20/03/26 16:19]:
> A vshard based cluster with 20 router nodes and 20 storage nodes,
> 32 core each, will have 20*32*32*16 connections.
These are only internal connections maintained by vshard.
Replication adds miniscule 32*20*2 connection to that.
Any monitoring will be likely establishing a new connection for
every monitoring request, 32*40 connections per second.
Any quality of service feature, like separating oltp and
analytical traffic, could easily require doubling it, since all
prioritized work has to use its own transport infrastructure.
Any network partitioning event will lead to avalanche like
re-establishment of these connections.
One can ignore performance concerns, of course, given there has
been a foundation of scrupulous accounting for every bit and every
CPU cycle, but not very long.
> A 20-40 node cluster is not big by any means.
>
> --
> Konstantin Osipov, Moscow, Russia
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-25 7:35 ` lvasiliev
2020-03-25 8:42 ` Konstantin Osipov
@ 2020-03-26 21:13 ` Alexander Turenko
2020-03-26 21:53 ` Alexander Turenko
2020-03-27 8:28 ` Konstantin Osipov
2020-03-26 23:35 ` Alexander Turenko
2 siblings, 2 replies; 30+ messages in thread
From: Alexander Turenko @ 2020-03-26 21:13 UTC (permalink / raw)
To: lvasiliev; +Cc: tarantool-patches
On Wed, Mar 25, 2020 at 10:35:41AM +0300, lvasiliev wrote:
>
>
> On 24.03.2020 23:02, Konstantin Osipov wrote:
> > * Leonid Vasiliev <lvasiliev@tarantool.org> [20/03/24 16:02]:
> > > The negotiation phase has been added to IPROTO
> > >
> > > For possibility to have a custom parameters of session the negotiation
> > > phase has been added. This is necessary to enable the transmission of
> > > an error in different formats(depending on the choice of the client).
> > >
> > > @TarantoolBot document
> > > Title: IPROTO: The negatiation phase
> > > For backward compatibility of the data transmission format,
> > > the negotiation phase has been added to IPROTO.
> > > A new key (IPROTO_NEGOTIATION) has been added to IPROTO command codes.
> > > NEGOTIATION BODY: CODE = 0x0E
> > > +==========================+
> > > | |
> > > | NEGOTIATION PARAMETERS |
> > > | |
> > > +==========================+
> > > MP_MAP
> > > Session negotiation parameters are a map with keys like ERROR_FORMAT_VERSION ...
> > > The response is a map with all stated negotiation parameters.
> > > So, for work with the new format of errors, it is necessary to perform the negotiation phase,
> > > otherwise errors will be transmitted in the old format (by default).
> >
> > Why not make it a key in IPROTO_AUTH, and require a separate
> > round-trip?
> Hi. Because it's not a part of AAA.
We discussed it a bit with Leonid and his main concern is naming. AUTH
is not about session settings.
We can rename the packet identifier to, say, ALTER_SESSION within our
code, but keep the code (a number) the same. (Alternative: introduce a
new ALTER_SESSION packet, which able to authorize and set session
parameters; don't sure what is better.)
(If we'll look into the variant with keeping the packet number.) This
ALTER_SESSION packet should be backward compatible with AUTH by the
request and response format (at least for responses for requests w/o
newly introduced fields).
ALTER_SESSION should support:
* Request: acquiring a specific value for a session parameter (any of
supported session sessings, not only one).
* Response: explicitly state that all requested settings were applied
(to distinguish old/new server responses, the old one will only do
authorization).
* Should support altering of session settings w/o (re)auth.
Is it looks okay for you, Leonid?
WBR, Alexander Turenko.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-26 21:13 ` Alexander Turenko
@ 2020-03-26 21:53 ` Alexander Turenko
2020-03-27 8:28 ` Konstantin Osipov
1 sibling, 0 replies; 30+ messages in thread
From: Alexander Turenko @ 2020-03-26 21:53 UTC (permalink / raw)
To: lvasiliev; +Cc: tarantool-patches
On Fri, Mar 27, 2020 at 12:13:43AM +0300, Alexander Turenko wrote:
> On Wed, Mar 25, 2020 at 10:35:41AM +0300, lvasiliev wrote:
> >
> >
> > On 24.03.2020 23:02, Konstantin Osipov wrote:
> > > * Leonid Vasiliev <lvasiliev@tarantool.org> [20/03/24 16:02]:
> > > > The negotiation phase has been added to IPROTO
> > > >
> > > > For possibility to have a custom parameters of session the negotiation
> > > > phase has been added. This is necessary to enable the transmission of
> > > > an error in different formats(depending on the choice of the client).
> > > >
> > > > @TarantoolBot document
> > > > Title: IPROTO: The negatiation phase
> > > > For backward compatibility of the data transmission format,
> > > > the negotiation phase has been added to IPROTO.
> > > > A new key (IPROTO_NEGOTIATION) has been added to IPROTO command codes.
> > > > NEGOTIATION BODY: CODE = 0x0E
> > > > +==========================+
> > > > | |
> > > > | NEGOTIATION PARAMETERS |
> > > > | |
> > > > +==========================+
> > > > MP_MAP
> > > > Session negotiation parameters are a map with keys like ERROR_FORMAT_VERSION ...
> > > > The response is a map with all stated negotiation parameters.
> > > > So, for work with the new format of errors, it is necessary to perform the negotiation phase,
> > > > otherwise errors will be transmitted in the old format (by default).
> > >
> > > Why not make it a key in IPROTO_AUTH, and require a separate
> > > round-trip?
> > Hi. Because it's not a part of AAA.
>
> We discussed it a bit with Leonid and his main concern is naming. AUTH
> is not about session settings.
>
> We can rename the packet identifier to, say, ALTER_SESSION within our
> code, but keep the code (a number) the same. (Alternative: introduce a
> new ALTER_SESSION packet, which able to authorize and set session
> parameters; don't sure what is better.)
>
> (If we'll look into the variant with keeping the packet number.) This
> ALTER_SESSION packet should be backward compatible with AUTH by the
> request and response format (at least for responses for requests w/o
> newly introduced fields).
>
> ALTER_SESSION should support:
>
> * Request: acquiring a specific value for a session parameter (any of
> supported session sessings, not only one).
> * Response: explicitly state that all requested settings were applied
> (to distinguish old/new server responses, the old one will only do
> authorization).
> * Should support altering of session settings w/o (re)auth.
>
> Is it looks okay for you, Leonid?
(After more discussions with Leonid and Sergos.)
However it seems that this extended (or new) package is purely for
optimization. When a new session parameter is added it is available via
_vsession_settings system space and a client may update it just after
AUTH.
Designing of ALTER_SESSION (a kind of AUTH+UPDATE _vsession_settings)
can be excluded from the scope of this task: it is the common mechanism,
which is applicable for all session settings.
A client should take care to wait all in-fly requests before send UPDATE
to _vsession_settings. It is quite simple, however, after connect and
AUTH: there are no in-fly requests at this time.
Aren't I miss anything?
WBR, Alexander Turenko.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-25 7:35 ` lvasiliev
2020-03-25 8:42 ` Konstantin Osipov
2020-03-26 21:13 ` Alexander Turenko
@ 2020-03-26 23:35 ` Alexander Turenko
2020-03-27 8:39 ` Konstantin Osipov
2 siblings, 1 reply; 30+ messages in thread
From: Alexander Turenko @ 2020-03-26 23:35 UTC (permalink / raw)
To: lvasiliev; +Cc: tarantool-patches
> > Why have it at all and not look at server version, which is part
> > of the greeting already?
> >
> The cause is backward compatibility.
> For example: a client application may expect an error as a string (IPROTO_OK
> case) and instead of which it will receive an error as an “object”. A
> greeting is sent only from the server side to the client, but the server
> must know what format should be used to send errors (what format does the
> client expect).
(After the discussion with Leonid and Vlad.)
The new idea appears: don't do any sessions setting / negotiation, but
extend IPROTO_OK response in the backward compatible way. (It is a bit
wild, I understood.)
Send everything inside IPROTO_BODY as usual: with errors as strings. Add
the new field into the response, say, IPROTO_ERROR_META. It is mp_map,
where keys are offset / path to strings, which were errors. Values are
maps, which will contain all necessary data to create full-featured
error: error code, backtrace, etc.
We possibly can even reuse the format from the stacked disgnostics RFC.
We can even strip error messages from this map to reduce traffic.
It will be tricky to collect this metainfo during serialization and it
will be tricky to replace strings with errors during deserialization.
But doable.
NB: Whether this mechanism can be reusable for other similar cases? It
looks too heavy in coding to do it just for errors.
WBR, Alexander Turenko.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-26 21:13 ` Alexander Turenko
2020-03-26 21:53 ` Alexander Turenko
@ 2020-03-27 8:28 ` Konstantin Osipov
1 sibling, 0 replies; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-27 8:28 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
* Alexander Turenko <alexander.turenko@tarantool.org> [20/03/27 11:14]:
I think the criteria for making the decision is checking that
clients can cope with extra fields in AUTH response map.
If they do, no need to add a new packet, simply use existing
packet after renaming.
Otherwise introduce a new one.
Please check auth implementation in iproto - AFAIR it flushes the
iproto_msg pipe, so is synchronous.
We also had a proposal to introduce stream identifiers, necessary
for interactive transactions. Perhaps we need to revive this
proposal to make sure we don't have to change the protocol one
more time to add streams, and introduce all the keys for streams
right away - it's also a session control statement.
> > > * Leonid Vasiliev <lvasiliev@tarantool.org> [20/03/24 16:02]:
> > > > The negotiation phase has been added to IPROTO
> > > >
> > > > For possibility to have a custom parameters of session the negotiation
> > > > phase has been added. This is necessary to enable the transmission of
> > > > an error in different formats(depending on the choice of the client).
> > > >
> > > > @TarantoolBot document
> > > > Title: IPROTO: The negatiation phase
> > > > For backward compatibility of the data transmission format,
> > > > the negotiation phase has been added to IPROTO.
> > > > A new key (IPROTO_NEGOTIATION) has been added to IPROTO command codes.
> > > > NEGOTIATION BODY: CODE = 0x0E
> > > > +==========================+
> > > > | |
> > > > | NEGOTIATION PARAMETERS |
> > > > | |
> > > > +==========================+
> > > > MP_MAP
> > > > Session negotiation parameters are a map with keys like ERROR_FORMAT_VERSION ...
> > > > The response is a map with all stated negotiation parameters.
> > > > So, for work with the new format of errors, it is necessary to perform the negotiation phase,
> > > > otherwise errors will be transmitted in the old format (by default).
> > >
> > > Why not make it a key in IPROTO_AUTH, and require a separate
> > > round-trip?
> > Hi. Because it's not a part of AAA.
>
> We discussed it a bit with Leonid and his main concern is naming. AUTH
> is not about session settings.
>
> We can rename the packet identifier to, say, ALTER_SESSION within our
> code, but keep the code (a number) the same. (Alternative: introduce a
> new ALTER_SESSION packet, which able to authorize and set session
> parameters; don't sure what is better.)
>
> (If we'll look into the variant with keeping the packet number.) This
> ALTER_SESSION packet should be backward compatible with AUTH by the
> request and response format (at least for responses for requests w/o
> newly introduced fields).
>
> ALTER_SESSION should support:
>
> * Request: acquiring a specific value for a session parameter (any of
> supported session sessings, not only one).
> * Response: explicitly state that all requested settings were applied
> (to distinguish old/new server responses, the old one will only do
> authorization).
> * Should support altering of session settings w/o (re)auth.
>
> Is it looks okay for you, Leonid?
>
> WBR, Alexander Turenko.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase
2020-03-26 23:35 ` Alexander Turenko
@ 2020-03-27 8:39 ` Konstantin Osipov
0 siblings, 0 replies; 30+ messages in thread
From: Konstantin Osipov @ 2020-03-27 8:39 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
* Alexander Turenko <alexander.turenko@tarantool.org> [20/03/27 11:14]:
> > > Why have it at all and not look at server version, which is part
> > > of the greeting already?
> > >
> > The cause is backward compatibility.
> > For example: a client application may expect an error as a string (IPROTO_OK
> > case) and instead of which it will receive an error as an “object”. A
> > greeting is sent only from the server side to the client, but the server
> > must know what format should be used to send errors (what format does the
> > client expect).
>
> (After the discussion with Leonid and Vlad.)
>
> The new idea appears: don't do any sessions setting / negotiation, but
> extend IPROTO_OK response in the backward compatible way. (It is a bit
> wild, I understood.)
>
> Send everything inside IPROTO_BODY as usual: with errors as strings. Add
> the new field into the response, say, IPROTO_ERROR_META. It is mp_map,
> where keys are offset / path to strings, which were errors. Values are
> maps, which will contain all necessary data to create full-featured
> error: error code, backtrace, etc.
>
> We possibly can even reuse the format from the stacked disgnostics RFC.
>
> We can even strip error messages from this map to reduce traffic.
>
> It will be tricky to collect this metainfo during serialization and it
> will be tricky to replace strings with errors during deserialization.
> But doable.
>
> NB: Whether this mechanism can be reusable for other similar cases? It
> looks too heavy in coding to do it just for errors.
>
It's a good idea to extend existing packets in backward compatible
manner if it can be done in a meaningful way and without breaking
old clients.
All tarantool clients should ignore unknown map keys.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Extending error functionality
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
` (5 preceding siblings ...)
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 6/6] error: Transmit an error through IPROTO_OK as object Leonid Vasiliev
@ 2020-03-27 23:11 ` lvasiliev
2020-03-28 13:54 ` Alexander Turenko
2020-04-01 15:35 ` Alexander Turenko
7 siblings, 1 reply; 30+ messages in thread
From: lvasiliev @ 2020-03-27 23:11 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
Summary of discussion with Alexander Turenko, Vlad, Mons, Sergos and
Kirill Yukhin:
1) Session parameters
Move negotiations to UPDATE (as for _vsession_settings).
As further optimization session settings can be moved to auth message
for to remove an additional RTT
2) Backtrace
Leave adding to the error only Lua backtrace (which is added to the
error at luaT_pusherror()).
We must have a per server option for enable/disable adding backtrace.
In addition a bactrace enable/disable adding can be forced by the
creation function parametr.
The backtrace should be combined with the message if tostring is
used.(@knazarov )
Notes:
To add a C bt is possible when error is created (for example) but unwind
the Lua stack in diag.c is difficult (reason: the lua functions like
lua_getstack, lua_getinfo are unavailable where)
3) About returning(return box.error.new(...)) an error through IPROTO
Now the error is serialized to string for marshaling through connection,
which makes it difficult to add various error parameters.
In the first iteration format was changed to table. But after discussion
was decided to add a new msgpack type for the errors transmitting(by
analogy with MP_DECIMAL)
An intermediate type mp_error will be introduced for this purposes
(reason: it is impossible to create an object of the ClientError class
(or some other classes) at the time of decoding)
Due to the need to use different serialization methods, depending on the
session settings, the serializator context (containing fickle options)
was added.
4) Payload
For storage the payload at struct error will be added ptr to data with
data size.
For using a payload from Lua set/get_payload (worked with table as
argument/return type) will be used. It is possible to use msgpack encode
when set and decode on get, but not is rationally.
To reduce overhead is offered to save a link to the table and use it as
response output.
For these purposes is proposed to use inheritance from the struct error
in the C style with adding additional attributes like ptr to the Lua
table. In the case of a marshaling through net the payload will be
encoded/decoded with msgpack using.
On 24.03.2020 15:45, Leonid Vasiliev wrote:
> https://github.com/tarantool/tarantool/issues/4398
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4398-expose-error-module-4
>
> According to https://github.com/tarantool/tarantool/issues/4398
> (and after some discussion) we would like box.error to have:
> * Ability to create new error types
> * Transparent marshalling through net.box
> * Lua backtrace
> * payload (not implemented in patch)
>
>
> Leonid Vasiliev (6):
> error: Add a Lua backtrace to error
> error: Add the custom error type
> iproto: Add negotiation phase
> error: Add extended error transfer format
> error: Add test for extended error
> error: Transmit an error through IPROTO_OK as object
>
> src/box/errcode.h | 1 +
> src/box/error.cc | 56 +++++++++
> src/box/error.h | 32 ++++++
> src/box/execute.c | 1 +
> src/box/iproto.cc | 40 ++++++-
> src/box/iproto_constants.h | 25 +++++
> src/box/lua/call.c | 29 +++--
> src/box/lua/error.cc | 126 +++++++++++++++++----
> src/box/lua/execute.c | 2 +-
> src/box/lua/net_box.c | 34 ++++++
> src/box/lua/net_box.lua | 103 +++++++++++++++--
> src/box/lua/tuple.c | 12 +-
> src/box/session.cc | 16 +++
> src/box/session.h | 20 ++++
> src/box/sql/func.c | 4 +-
> src/box/xrow.c | 160 ++++++++++++++++++++++++++
> src/box/xrow.h | 48 +++++++-
> src/lib/core/diag.c | 20 ++++
> src/lib/core/diag.h | 5 +
> src/lib/core/exception.cc | 1 +
> src/lib/core/mp_extension_types.h | 4 +-
> src/lib/core/port.h | 4 +-
> src/lua/error.c | 45 +++++++-
> src/lua/error.h | 6 +-
> src/lua/error.lua | 65 ++++++++---
> src/lua/msgpack.c | 26 +++--
> src/lua/msgpack.h | 8 +-
> src/lua/utils.c | 28 ++++-
> src/lua/utils.h | 27 ++++-
> test/app/fiber.result | 35 ++++--
> test/app/fiber.test.lua | 11 +-
> test/box-tap/extended_error.test.lua | 212 +++++++++++++++++++++++++++++++++++
> test/box/misc.result | 51 +++++++--
> test/box/misc.test.lua | 19 +++-
> 34 files changed, 1161 insertions(+), 115 deletions(-)
> create mode 100755 test/box-tap/extended_error.test.lua
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Extending error functionality
2020-03-27 23:11 ` [Tarantool-patches] [PATCH 0/6] Extending error functionality lvasiliev
@ 2020-03-28 13:54 ` Alexander Turenko
2020-03-30 10:48 ` lvasiliev
0 siblings, 1 reply; 30+ messages in thread
From: Alexander Turenko @ 2020-03-28 13:54 UTC (permalink / raw)
To: lvasiliev; +Cc: tarantool-patches, Vladislav Shpilevoy
Thanks for summarization!
I'll add my two coins.
CCed all participants.
WBR, Alexander Turenko.
On Sat, Mar 28, 2020 at 02:11:46AM +0300, lvasiliev wrote:
> Summary of discussion with Alexander Turenko, Vlad, Mons, Sergos and Kirill
> Yukhin:
> 1) Session parameters
> Move negotiations to UPDATE (as for _vsession_settings).
To be clear: use UPDATE of _vsession_settings instead of the negotiation
protocol. It is less intruisive. Also it looks logical to have all
session settings in _vsession_settings.
> As further optimization session settings can be moved to auth message for to
> remove an additional RTT
However I don't sure whether it is really worth, because we can send two
packets in a batch (send the second w/o waiting for answer of the first
one). A client should handle four cases: {success, failure} x {success,
failure} in the case: it does not look difficult.
>
> 2) Backtrace
> Leave adding to the error only Lua backtrace (which is added to the error at
> luaT_pusherror()).
> We must have a per server option for enable/disable adding backtrace.
> In addition a bactrace enable/disable adding can be forced by the creation
> function parametr.
Ok.
> The backtrace should be combined with the message if tostring is
> used.(@knazarov )
We didn't discuss it on our meeting and, to be honest, I unsure here.
AFAIU, Kostya want to see a backtrace in logs when an error is not
catched anywhere (and maybe in some other cases?). We should ask for
expected behaviour rather that for certain implementation.
We have __tostring(), which is used when casting to a string explicitly
or implicitly via concatenation. I would expect just error message here.
We have __serialize() for repl. It is logical to have backtrace and
other fields here.
Note: __serialize() returns an error message and on the first glance it
seems that we should keep it here for the old way marshaling over the
binary protocol. However, no, we'll handle errors in serializers
explicitly and will not use __serialize().
What method is in use when an error is written to logs (when not
catched, when diag_log() is used, when written by log.info() from Lua)?
I think it should be __serialize(), but it is possible that it is not so
now.
Hm. Maybe Kostya is about case like:
| local res, err = my_func()
| if res == nil then
| log.info('My message, reason: ' .. err)
| return
| end
What we'll propose for this case? Something like so?
| log.info('My message, reason: ' .. yaml.encode(err))
Need to think.
> Notes:
> To add a C bt is possible when error is created (for example) but unwind the
> Lua stack in diag.c is difficult (reason: the lua functions like
> lua_getstack, lua_getinfo are unavailable where)
To be clear: C stack trace itself maybe not so valuabe, when it is not
intermixed with Lua frames information. The case is using of a trigger.
So we agreed to not add them, at least for now.
>
> 3) About returning(return box.error.new(...)) an error through IPROTO
> Now the error is serialized to string for marshaling through connection,
> which makes it difficult to add various error parameters.
> In the first iteration format was changed to table. But after discussion was
> decided to add a new msgpack type for the errors transmitting(by analogy
> with MP_DECIMAL)
Note: The new mp_exp subtype should have extensible format. I propose to
keep key-value pairs like in mp_map or even contain mp_map inside. Keys
should be number constants, not strings (to shrink packet size).
> An intermediate type mp_error will be introduced for this purposes (reason:
> it is impossible to create an object of the ClientError class (or some
> other classes) at the time of decoding)
Seems ok.
However I would note that rewriting all error into C is also possible
way to solve this.
> Due to the need to use different serialization methods, depending on the
> session settings, the serializator context (containing fickle options) was
> added.
Seems ok. Maybe.
However I still have some doubt about dividing options to 'rarely
changed' and 'often changed'. It looks highly dependent of use case and
does not looks as property of an option.
And it seems that it is necessary only to save copying of ~60-80 bytes
of `struct serializer` for each serialization process.
Should not this context allow to store any serializer option? Or maybe
we should provide some common way to speed up options 'copying'. Or
maybe this <100 bytes copy is not so heavy?
Dividing options to rarely/often changed feels as not general solution
of the problem for me and that is why I complain.
Maybe we can use caching here? Let's calculate hashsum of per-call
serializer options and store full set of serializer options in a
hashmap. When global options are changed, this hashmap should be flushed
or updated.
This looks as general solution: it works for any option, it speeds up
(at least in theory) per-call options usage.
TL;DR: Idea is to cache full serializer options using per-call
serializer options (subset of all options).
>
> 4) Payload
> For storage the payload at struct error will be added ptr to data with data
> size.
> For using a payload from Lua set/get_payload (worked with table as
> argument/return type) will be used. It is possible to use msgpack encode
> when set and decode on get, but not is rationally.
> To reduce overhead is offered to save a link to the table and use it as
> response output.
> For these purposes is proposed to use inheritance from the struct error in
> the C style with adding additional attributes like ptr to the Lua table. In
> the case of a marshaling through net the payload will be encoded/decoded
> with msgpack using.
Note: We should move all additional fields from error C++ classes to
struct error (use union) and remove all C++ virtual methods from those
classes. After this we can add more fields to a `struct error` child w/o
padding.
Note: We should return a pointer to base (`struct error`) to Lua to keep
cdata type being `struct error&`. It seems that iserror() is not
documented and users likely use ffi.istype().
We agreed on serializing Lua payload into msgpack at encoding of an
error (rather than doing it at an error creation). A table of virtual
methods should be added so to serialize base error / lua error into
msgpack (like it is done in `struct port`).
The reason to postpone serialization of payload to an error
serialization is that we don't necessarily will serialize an error at
all: maybe it'll be processed within a tarantool instance.
WBR, Alexander Turenko.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Extending error functionality
2020-03-28 13:54 ` Alexander Turenko
@ 2020-03-30 10:48 ` lvasiliev
0 siblings, 0 replies; 30+ messages in thread
From: lvasiliev @ 2020-03-30 10:48 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches, Vladislav Shpilevoy
On 28.03.2020 16:54, Alexander Turenko wrote:
> Thanks for summarization!
>
> I'll add my two coins.
>
> CCed all participants.
>
> WBR, Alexander Turenko.
>
> On Sat, Mar 28, 2020 at 02:11:46AM +0300, lvasiliev wrote:
>> Summary of discussion with Alexander Turenko, Vlad, Mons, Sergos and Kirill
>> Yukhin:
>> 1) Session parameters
>> Move negotiations to UPDATE (as for _vsession_settings).
>
> To be clear: use UPDATE of _vsession_settings instead of the negotiation
> protocol. It is less intruisive. Also it looks logical to have all
> session settings in _vsession_settings.
>
>> As further optimization session settings can be moved to auth message for to
>> remove an additional RTT
>
> However I don't sure whether it is really worth, because we can send two
> packets in a batch (send the second w/o waiting for answer of the first
> one). A client should handle four cases: {success, failure} x {success,
> failure} in the case: it does not look difficult.
>
>>
>> 2) Backtrace
>> Leave adding to the error only Lua backtrace (which is added to the error at
>> luaT_pusherror()).
>> We must have a per server option for enable/disable adding backtrace.
>> In addition a bactrace enable/disable adding can be forced by the creation
>> function parametr.
>
> Ok.
>
>> The backtrace should be combined with the message if tostring is
>> used.(@knazarov )
>
> We didn't discuss it on our meeting and, to be honest, I unsure here.
>
> AFAIU, Kostya want to see a backtrace in logs when an error is not
> catched anywhere (and maybe in some other cases?). We should ask for
> expected behaviour rather that for certain implementation.
>
> We have __tostring(), which is used when casting to a string explicitly
> or implicitly via concatenation. I would expect just error message here.
>
> We have __serialize() for repl. It is logical to have backtrace and
> other fields here.
>
> Note: __serialize() returns an error message and on the first glance it
> seems that we should keep it here for the old way marshaling over the
> binary protocol. However, no, we'll handle errors in serializers
> explicitly and will not use __serialize().
>
> What method is in use when an error is written to logs (when not
> catched, when diag_log() is used, when written by log.info() from Lua)?
> I think it should be __serialize(), but it is possible that it is not so
> now.
>
> Hm. Maybe Kostya is about case like:
>
> | local res, err = my_func()
> | if res == nil then
> | log.info('My message, reason: ' .. err)
> | return
> | end
>
> What we'll propose for this case? Something like so?
>
> | log.info('My message, reason: ' .. yaml.encode(err))
>
> Need to think.I will copy it to https://github.com/tarantool/tarantool/issues/4398
because Kostya doesn't read the mailing list.
>
>> Notes:
>> To add a C bt is possible when error is created (for example) but unwind the
>> Lua stack in diag.c is difficult (reason: the lua functions like
>> lua_getstack, lua_getinfo are unavailable where)
>
> To be clear: C stack trace itself maybe not so valuabe, when it is not
> intermixed with Lua frames information. The case is using of a trigger.
> So we agreed to not add them, at least for now.
>
>>
>> 3) About returning(return box.error.new(...)) an error through IPROTO
>> Now the error is serialized to string for marshaling through connection,
>> which makes it difficult to add various error parameters.
>> In the first iteration format was changed to table. But after discussion was
>> decided to add a new msgpack type for the errors transmitting(by analogy
>> with MP_DECIMAL)
>
> Note: The new mp_exp subtype should have extensible format. I propose to
> keep key-value pairs like in mp_map or even contain mp_map inside. Keys
> should be number constants, not strings (to shrink packet size).
>
>> An intermediate type mp_error will be introduced for this purposes (reason:
>> it is impossible to create an object of the ClientError class (or some
>> other classes) at the time of decoding)
>
> Seems ok.
>
> However I would note that rewriting all error into C is also possible
> way to solve this.
In my mind it depends on how it will be implemented on C. Now you
propose to move some additional details from module to the base
implementation with a decrease of flexibility. All new implementations
of errors will affect the Base.
>
>> Due to the need to use different serialization methods, depending on the
>> session settings, the serializator context (containing fickle options) was
>> added.
>
> Seems ok. Maybe.
>
> However I still have some doubt about dividing options to 'rarely
> changed' and 'often changed'. It looks highly dependent of use case and
> does not looks as property of an option.
>
> And it seems that it is necessary only to save copying of ~60-80 bytes
> of `struct serializer` for each serialization process.
>
> Should not this context allow to store any serializer option? Or maybe
> we should provide some common way to speed up options 'copying'. Or
> maybe this <100 bytes copy is not so heavy?
>
> Dividing options to rarely/often changed feels as not general solution
> of the problem for me and that is why I complain.
>
> Maybe we can use caching here? Let's calculate hashsum of per-call
> serializer options and store full set of serializer options in a
> hashmap. When global options are changed, this hashmap should be flushed
> or updated.
>
> This looks as general solution: it works for any option, it speeds up
> (at least in theory) per-call options usage.
>
> TL;DR: Idea is to cache full serializer options using per-call
> serializer options (subset of all options).
>
In my opinion it looks like
https://refactoring.guru/design-patterns/flyweight. I think
implementation with cache is overkill and overhead.
>>
>> 4) Payload
>> For storage the payload at struct error will be added ptr to data with data
>> size.
>> For using a payload from Lua set/get_payload (worked with table as
>> argument/return type) will be used. It is possible to use msgpack encode
>> when set and decode on get, but not is rationally.
>> To reduce overhead is offered to save a link to the table and use it as
>> response output.
>> For these purposes is proposed to use inheritance from the struct error in
>> the C style with adding additional attributes like ptr to the Lua table. In
>> the case of a marshaling through net the payload will be encoded/decoded
>> with msgpack using.
>
> Note: We should move all additional fields from error C++ classes to
> struct error (use union) and remove all C++ virtual methods from those
> classes. After this we can add more fields to a `struct error` child w/o
> padding.
>
> Note: We should return a pointer to base (`struct error`) to Lua to keep
> cdata type being `struct error&`. It seems that iserror() is not
> documented and users likely use ffi.istype().
>
> We agreed on serializing Lua payload into msgpack at encoding of an
> error (rather than doing it at an error creation). A table of virtual
> methods should be added so to serialize base error / lua error into
> msgpack (like it is done in `struct port`).
>
> The reason to postpone serialization of payload to an error
> serialization is that we don't necessarily will serialize an error at
> all: maybe it'll be processed within a tarantool instance.
>
I thought about this variant after discussion.
Now we have a structured, readable, self-documented implementation of
the error hierarchy. If we will using the union, it will look like ugly
"all in one" (struct in union will be splitting in the case of adding a
new type of error) and will be unreadable without comments. In addition,
this variant will be very fragile (multiple inheritance). What do you
think about implementation with adding to struct error void pointer with
enum type_of_payload (variation of your original propose)?
> WBR, Alexander Turenko.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/6] Extending error functionality
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
` (6 preceding siblings ...)
2020-03-27 23:11 ` [Tarantool-patches] [PATCH 0/6] Extending error functionality lvasiliev
@ 2020-04-01 15:35 ` Alexander Turenko
7 siblings, 0 replies; 30+ messages in thread
From: Alexander Turenko @ 2020-04-01 15:35 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches, Vladislav Shpilevoy
We discussed several questions around the API and the implementation
yesterday with Leonid and Vlad. Vlad summarized the API points in the
issue (see [1]). I want to summarize implementation details we
more-or-less agreed on.
We have two places, where encoding and decoding is performed to/from
msgpack: they are src/lua/{msgpack.c,msgpackffi.lua}.
I propose to add two mappings to msgpack.c:
| /*
| * Encode an object on @an idx slot on the Lua stack into
| * @a buf, promote @a buf pointer on written bytes amount.
| *
| * Don't write over @an end pointer (inclusive).
| */
| typedef void (*encode_ext_f)(struct lua_State *L, int idx, char **buf,
| char *end);
|
| /*
| * Decode msgpack from @buf, push it onto the Lua stack.
| *
| * Promote @a buf on amount of read bytes.
| *
| * Don't read over @an end (inclusive).
| *
| */
| typedef void (*decode_ext_f)(struct lua_State *L, char **buf, char *end);
|
| /* cdata type id -> encode function */
| static mh_encode_ext;
|
| /* msgpack extension type -> decode function */
| static mh_decode_ext;
|
| /* Register encode function for a cdata type. */
| void
| register_encode_ext(int ctypeid, encode_ext_f f);
|
| /* Register decode function for an extension type. */
| void
| register_decode_ext(int mp_ext_type, decode_ext_f f);
So box will add its own encode / decode functions for box.error objects
on box initialization (at tarantool loading, not at box.cfg()). The
problem we'll solve here is that src/lua/msgpack.c should not be
statically linked with src/box.
The similar change should be made for msgpackffi.lua. It, however,
already has `encode_ext_cdata` and `ext_decoder` tables. So it seems we
just need to add registration functions.
Note: Don't sure whether encode_ext / decode_ext should operate directly on
the Lua stack or we should separate those functions into get_from_stack+encode
and decode+push_to_stack. (Vlad?)
The functions that will be exposed by box should encode any box.error
object into msgpack and decode any msgpacked box.error into box.error
object. Build*Error() constructors should be called for this with
appropriate arguments. So the decode function will contain the switch by
an error types and call necessary Build*Error() functions. Maybe also
will replace the error message, where needed.
This way looks hacky, but at least it concentrates hacking in one place.
When we'll remove C++ classes for errors, we can refactor this part.
[1]: https://github.com/tarantool/tarantool/issues/4398#issuecomment-606937494
WBR, Alexander Turenko.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/6] error: Add the custom error type
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 2/6] error: Add the custom error type Leonid Vasiliev
@ 2020-04-05 22:14 ` Vladislav Shpilevoy
0 siblings, 0 replies; 30+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-05 22:14 UTC (permalink / raw)
To: Leonid Vasiliev, alexander.turenko; +Cc: tarantool-patches
Thanks for the patch!
See 6 comments below.
On 24/03/2020 13:46, Leonid Vasiliev wrote:
> A possibility to create an error with a custom subtype was added.
>
> In accordance with https://github.com/tarantool/tarantool/issues/4398
> a custom error type has been added to the box.error.
> Now, it's possible to create an error with a custom subtype (string value)
> for use it in applications.
>
> @TarantoolBot document
> Title: error.custom_type
> A custom error type has been added to the box.error.
> Now, it's possible to create an error with a custom subtype (string value)
> for use it in applications.
>
> Example:
>
> err_custom = box.error.new(box.error.CUSTOM_ERROR, "My Custom Type", "Reason")
> Now:
> err_custom.type == "CustomError"
> err_custom.custom_type == "My Custom Type"
1. Regarding types we discussed that verbally and then I
wrote it in the ticket, that better return custom type in
err.type. And return the original type in a new field. To
simplify cascade error checking, when one type is compared
to many possible types to make appropriate actions. Just for
record.
> err_custom.message == "User custom error: Reason"
2. The whole point of having custom errors - be fully customizable.
I don't think it is a good idea to seal this prefix into every
user defined error. After all, usually errors are created not for
end users, but for systems built on top of tarantool. Hence it
looks odd to even mention 'User'.
3. In the doc request you didn't say a word about custom error
type length limit.
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 47dce33..25e7eff 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -253,3 +274,38 @@ 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 ?: "");
> +
> + if (custom_type) {
> + strncpy(m_custom_type, custom_type, 63);
> + m_custom_type[63] = '\0';
4. Please, don't do that. Never use constants the same constant
in several places, and depending on other constants. Use
sizeof(m_custom_type) - 1.
> + } else {
> + m_custom_type[0] = '\0';
> + }
> +}
> diff --git a/src/box/error.h b/src/box/error.h
> index b8c7cf7..3e0beb8 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -70,6 +73,14 @@ const char *
> box_error_type(const box_error_t *error);
>
> /**
> + * Return the error custom type,
> + * \param error
> + * \return pointer to custom error type. On error, return NULL
> + */
> +const char *
> +box_custom_error_type(const box_error_t *e);
> +
5. Please, move this out of '\cond public'. I don't think
we are ready to release custom errors as part of the public
C API. The same for box_custom_error_set().
> +/**
> * Return IPROTO error code
> * \param error error
> * \return enum box_error_code
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 765ce73..26abec8 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -148,11 +152,16 @@ local function error_serialize(err)
> return error_message(err)
> end
>
> +local function error_is_custom(err)
> + return ffi.string(err._type.name) == 'CustomError'
> +end
> +
> local error_methods = {
> - ["unpack"] = error_unpack;
> - ["raise"] = error_raise;
> - ["match"] = error_match; -- Tarantool 1.6 backward compatibility
> - ["__serialize"] = error_serialize;
> + ["unpack"] = error_unpack,
> + ["raise"] = error_raise,
> + ["match"] = error_match, -- Tarantool 1.6 backward compatibility
> + ["is_custom"] = error_is_custom,
> + ["__serialize"] = error_serialize
6. The same as in the previous patch - try not to change
existing code when possible. Here it is possible. And below too.
> }
>
> local function error_index(err, key)
> @@ -181,11 +190,11 @@ local function error_concat(lhs, rhs)
> end
>
> local error_mt = {
> - __index = error_index;
> - __tostring = error_message;
> - __concat = error_concat;
> + __index = error_index,
> + __tostring = error_message,
> + __concat = error_concat
> };
>
> -ffi.metatype('struct error', error_mt);
> +ffi.metatype('struct error', error_mt)
>
> return error
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error
2020-03-24 12:45 ` [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error Leonid Vasiliev
@ 2020-04-05 22:14 ` Vladislav Shpilevoy
2020-04-08 13:56 ` Igor Munkin
0 siblings, 1 reply; 30+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-05 22:14 UTC (permalink / raw)
To: Leonid Vasiliev, alexander.turenko; +Cc: tarantool-patches
Thanks for the patch!
When you will work on the new patchset version, don't forget
to rebase on the latest master. Lots of things changed in errors.
See 7 comments below.
On 24/03/2020 13:45, Leonid Vasiliev wrote:
> Lua bactrace was added to a tarantool error.
>
> In accordance with https://github.com/tarantool/tarantool/issues/4398
> we want to have a Lua backtrace for the box.error
>
> @TarantoolBot document
> Title: error.bt
>
> Lua backtrace was added to a tarantool error.
>
> Needed for #4398
> ---
> diff --git a/src/lua/error.c b/src/lua/error.c
> index d82e78d..3a12e20 100644
> --- a/src/lua/error.c
> +++ b/src/lua/error.c
> @@ -34,8 +34,44 @@
> #include <fiber.h>
> #include "utils.h"
>
> +#include <string.h>
> +
> static int CTID_CONST_STRUCT_ERROR_REF = 0;
>
> +/*
> + * Memory for the traceback string is obtained with malloc,
> + * and can be freed with free.
> +*/
> +static char*
> +traceback (lua_State *L) {
1. Please, rename to lua_traceback(), remove whitespace between
function name and arguments, add 'struct' before lua_State,
add whitespace after 'char', add second '*' in the begining of
the comment, because we always use /** for comments not inside of
a function body.
Also the comment is not really useful. It does not say which trace
it collects - Lua, C, both? Does it resolve symbols in case of C,
or pushes raw addresses?
> + int top = lua_gettop(L);
> +
> + lua_getfield(L, LUA_GLOBALSINDEX, "debug");
> + if (!lua_istable(L, -1)) {
> + lua_settop(L, top);
> + return NULL;
> + }
> + lua_getfield(L, -1, "traceback");
> + if (!lua_isfunction(L, -1)) {
> + lua_settop(L, top);
> + return NULL;
> + }
> +
> + // call debug.traceback
2. Sorry, omit such comments please. They are really useless.
It is like to say:
// Condition is true if it is true.
if (true) {
// Condition was true.
...
Also please ask Igor if it is possible to get Lua traceback
from C without accessing LUA_GLOBALSINDEX, 'debug.traceback'.
> + lua_call(L, 0, 1);
> +
> + // get result of the debug.traceback call
> + if (!lua_isstring(L, -1)) {
> + lua_settop(L, top);
> + return NULL;
> + }
> +
> + char *bt = strdup(lua_tostring(L, -1));
> + lua_settop(L, top);
> +
> + return bt;
> +}
> +
> struct error *
> luaL_iserror(struct lua_State *L, int narg)
> {
> @@ -85,6 +121,13 @@ luaT_pusherror(struct lua_State *L, struct error *e)
> * then set the finalizer.
> */
> error_ref(e);
> +
> + if (e->lua_bt == NULL) {
> + char *lua_bt = traceback(L);
> + error_set_lua_bt(e, lua_bt);
3. This is double copying. traceback() uses strdup(),
error_set_lua_bt() copies onto a realloced string again.
You could pass traceback result directly to error_set_lua_bt().
> + free(lua_bt);
> + }
> +
> assert(CTID_CONST_STRUCT_ERROR_REF != 0);
> struct error **ptr = (struct error **)
> luaL_pushcdata(L, CTID_CONST_STRUCT_ERROR_REF);
> diff --git a/src/lua/error.h b/src/lua/error.h
> index 64fa5eb..16cdaf7 100644
> --- a/src/lua/error.h
> +++ b/src/lua/error.h
> @@ -65,6 +65,9 @@ luaT_pusherror(struct lua_State *L, struct error *e);
> struct error *
> luaL_iserror(struct lua_State *L, int narg);
>
> +struct error *
> +luaL_checkerror(struct lua_State *L, int narg);
4. Why? This function is still used only inside lua/error.c.
> +
> void
> tarantool_lua_error_init(struct lua_State *L);
>
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 7f24986..765ce73 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua> @@ -83,10 +84,18 @@ local function error_trace(err)
> return {}
> end
> return {
> - { file = ffi.string(err._file), line = tonumber(err._line) };
> + { file = ffi.string(err._file), line = tonumber(err._line) }
5. Please, omit not necessary diff.
> }
> end
>
> +local function error_backtrace(err)
> + local result = "Backtrace is absent"
> + if err.lua_bt ~= ffi.nullptr then
> + result = ffi.string(err.lua_bt)
> + end
> + return result
> +end
> +
> local function error_errno(err)
> local e = err._saved_errno
> if e == 0 then
> @@ -96,10 +105,11 @@ local function error_errno(err)
> end
>
> local error_fields = {
> - ["type"] = error_type;
> - ["message"] = error_message;
> - ["trace"] = error_trace;
> - ["errno"] = error_errno;
> + ["type"] = error_type,
> + ["message"] = error_message,
> + ["trace"] = error_trace,
> + ["errno"] = error_errno,
> + ["bt"] = error_backtrace
6. Please, keep the old fields with ';' to avoid
unnecessary changes. It does not prevent you from
adding backtrace.
Also seems like 'bt' is the only contraction here. Please,
use a full name. I thing traceback would be fine, similar
to debug.traceback. Although backtrace also looks good.
The only thing is that you should be consistent. And
currently you use both backtrace and traceback in different
places.
> }
>
> local function error_unpack(err)
> diff --git a/test/app/fiber.result b/test/app/fiber.result
> index 7331f61..3908733 100644
> --- a/test/app/fiber.result
> +++ b/test/app/fiber.result
> @@ -1036,14 +1036,33 @@ st;
> ---
> - false
> ...
> -e:unpack();
> ----
> -- type: ClientError
> - code: 1
> - message: Illegal parameters, oh my
> - trace:
> - - file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
> - line: 1
> +unpack_res = e:unpack();
7. Please, introduce new tests. Don't change existing. The
same for misc.test.lua.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error
2020-04-05 22:14 ` Vladislav Shpilevoy
@ 2020-04-08 13:56 ` Igor Munkin
0 siblings, 0 replies; 30+ messages in thread
From: Igor Munkin @ 2020-04-08 13:56 UTC (permalink / raw)
To: Leonid Vasiliev, Vladislav Shpilevoy; +Cc: tarantool-patches
Leonid,
Thanks for the patch! It's not a review, I'm just replying the comment I
was mentioned in (Vlad, feel free to CC me next time, I'm totally OK
with it).
On 06.04.20, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> When you will work on the new patchset version, don't forget
> to rebase on the latest master. Lots of things changed in errors.
>
> See 7 comments below.
>
> On 24/03/2020 13:45, Leonid Vasiliev wrote:
> > Lua bactrace was added to a tarantool error.
> >
> > In accordance with https://github.com/tarantool/tarantool/issues/4398
> > we want to have a Lua backtrace for the box.error
> >
> > @TarantoolBot document
> > Title: error.bt
> >
> > Lua backtrace was added to a tarantool error.
> >
> > Needed for #4398
> > ---
> > diff --git a/src/lua/error.c b/src/lua/error.c
> > index d82e78d..3a12e20 100644
> > --- a/src/lua/error.c
> > +++ b/src/lua/error.c
> > @@ -34,8 +34,44 @@
> > #include <fiber.h>
> > #include "utils.h"
> >
> > +#include <string.h>
> > +
> > static int CTID_CONST_STRUCT_ERROR_REF = 0;
> >
> > +/*
> > + * Memory for the traceback string is obtained with malloc,
> > + * and can be freed with free.
> > +*/
> > +static char*
> > +traceback (lua_State *L) {
>
> 1. Please, rename to lua_traceback(),
Oh, God, please, no!!! lua_* and luaL_* are Lua standart namespaces,
please stop spoiling them (even if the function is localized within a
translation unit and not exported to public API). Furthermore, it
doesn't respect lua_CFunction signature and lua_* prefix is definitely
a misguiding one here. Let's try to use other letters here for prefix.
> remove whitespace between function name and arguments, add 'struct'
> before lua_State, add whitespace after 'char', add second '*' in the
> begining of the comment, because we always use /** for comments not
> inside of a function body.
>
> Also the comment is not really useful. It does not say which trace
> it collects - Lua, C, both? Does it resolve symbols in case of C,
> or pushes raw addresses?
>
> > + int top = lua_gettop(L);
> > +
> > + lua_getfield(L, LUA_GLOBALSINDEX, "debug");
> > + if (!lua_istable(L, -1)) {
> > + lua_settop(L, top);
> > + return NULL;
> > + }
> > + lua_getfield(L, -1, "traceback");
> > + if (!lua_isfunction(L, -1)) {
> > + lua_settop(L, top);
> > + return NULL;
> > + }
> > +
> > + // call debug.traceback
>
> 2. Sorry, omit such comments please. They are really useless.
> It is like to say:
>
> // Condition is true if it is true.
> if (true) {
> // Condition was true.
> ...
>
> Also please ask Igor if it is possible to get Lua traceback
> from C without accessing LUA_GLOBALSINDEX, 'debug.traceback'.
There is a Lua-C API analogue for <debug.traceback>: luaL_traceback[1].
It was introduced in Lua 5.2 but was backported to LuaJIT and requires
no additional build flags to be enabled.
Unfortunately, I'm totally out of context, so I'm curious what prevents
us to inherit and adjust already implemented machinery presented in
<fiber_backtrace_cb>?
>
> > + lua_call(L, 0, 1);
> > +
> > + // get result of the debug.traceback call
> > + if (!lua_isstring(L, -1)) {
> > + lua_settop(L, top);
> > + return NULL;
> > + }
> > +
> > + char *bt = strdup(lua_tostring(L, -1));
Minor: it's not the safe way to obtain Lua string data (I've already
mentioned it here[2]).
> > + lua_settop(L, top);
> > +
> > + return bt;
> > +}
> > +
> > struct error *
> > luaL_iserror(struct lua_State *L, int narg)
> > {
> > @@ -85,6 +121,13 @@ luaT_pusherror(struct lua_State *L, struct error *e)
> > * then set the finalizer.
> > */
> > error_ref(e);
> > +
> > + if (e->lua_bt == NULL) {
> > + char *lua_bt = traceback(L);
> > + error_set_lua_bt(e, lua_bt);
>
> 3. This is double copying. traceback() uses strdup(),
> error_set_lua_bt() copies onto a realloced string again.
> You could pass traceback result directly to error_set_lua_bt().
>
> > + free(lua_bt);
> > + }
> > +
> > assert(CTID_CONST_STRUCT_ERROR_REF != 0);
> > struct error **ptr = (struct error **)
> > luaL_pushcdata(L, CTID_CONST_STRUCT_ERROR_REF);
> > diff --git a/src/lua/error.h b/src/lua/error.h
> > index 64fa5eb..16cdaf7 100644
> > --- a/src/lua/error.h
> > +++ b/src/lua/error.h
> > @@ -65,6 +65,9 @@ luaT_pusherror(struct lua_State *L, struct error *e);
> > struct error *
> > luaL_iserror(struct lua_State *L, int narg);
> >
> > +struct error *
> > +luaL_checkerror(struct lua_State *L, int narg);
One more time: please use another prefix (especially for an extern API).
>
> 4. Why? This function is still used only inside lua/error.c.
>
> > +
> > void
> > tarantool_lua_error_init(struct lua_State *L);
> >
> > diff --git a/src/lua/error.lua b/src/lua/error.lua
> > index 7f24986..765ce73 100644
> > --- a/src/lua/error.lua
> > +++ b/src/lua/error.lua> @@ -83,10 +84,18 @@ local function error_trace(err)
> > return {}
> > end
> > return {
> > - { file = ffi.string(err._file), line = tonumber(err._line) };
> > + { file = ffi.string(err._file), line = tonumber(err._line) }
>
> 5. Please, omit not necessary diff.
>
> > }
> > end
> >
> > +local function error_backtrace(err)
> > + local result = "Backtrace is absent"
> > + if err.lua_bt ~= ffi.nullptr then
> > + result = ffi.string(err.lua_bt)
> > + end
> > + return result
> > +end
> > +
> > local function error_errno(err)
> > local e = err._saved_errno
> > if e == 0 then
> > @@ -96,10 +105,11 @@ local function error_errno(err)
> > end
> >
> > local error_fields = {
> > - ["type"] = error_type;
> > - ["message"] = error_message;
> > - ["trace"] = error_trace;
> > - ["errno"] = error_errno;
> > + ["type"] = error_type,
> > + ["message"] = error_message,
> > + ["trace"] = error_trace,
> > + ["errno"] = error_errno,
> > + ["bt"] = error_backtrace
>
> 6. Please, keep the old fields with ';' to avoid
> unnecessary changes. It does not prevent you from
> adding backtrace.
>
> Also seems like 'bt' is the only contraction here. Please,
> use a full name. I thing traceback would be fine, similar
> to debug.traceback. Although backtrace also looks good.
> The only thing is that you should be consistent. And
> currently you use both backtrace and traceback in different
> places.
>
> > }
> >
> > local function error_unpack(err)
> > diff --git a/test/app/fiber.result b/test/app/fiber.result
> > index 7331f61..3908733 100644
> > --- a/test/app/fiber.result
> > +++ b/test/app/fiber.result
> > @@ -1036,14 +1036,33 @@ st;
> > ---
> > - false
> > ...
> > -e:unpack();
> > ----
> > -- type: ClientError
> > - code: 1
> > - message: Illegal parameters, oh my
> > - trace:
> > - - file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
> > - line: 1
> > +unpack_res = e:unpack();
>
> 7. Please, introduce new tests. Don't change existing. The
> same for misc.test.lua.
[1]: https://www.lua.org/manual/5.2/manual.html#luaL_traceback
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015498.html
--
Best regards,
IM
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2020-04-08 14:03 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
2020-03-24 12:45 ` [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error Leonid Vasiliev
2020-04-05 22:14 ` Vladislav Shpilevoy
2020-04-08 13:56 ` Igor Munkin
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 2/6] error: Add the custom error type Leonid Vasiliev
2020-04-05 22:14 ` Vladislav Shpilevoy
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase Leonid Vasiliev
2020-03-24 20:02 ` Konstantin Osipov
2020-03-25 7:35 ` lvasiliev
2020-03-25 8:42 ` Konstantin Osipov
2020-03-25 10:56 ` Eugene Leonovich
2020-03-25 11:13 ` Konstantin Osipov
2020-03-26 11:37 ` lvasiliev
2020-03-26 11:18 ` lvasiliev
2020-03-26 12:16 ` Konstantin Osipov
2020-03-26 12:54 ` Kirill Yukhin
2020-03-26 13:19 ` Konstantin Osipov
2020-03-26 13:31 ` Konstantin Osipov
2020-03-26 21:13 ` Alexander Turenko
2020-03-26 21:53 ` Alexander Turenko
2020-03-27 8:28 ` Konstantin Osipov
2020-03-26 23:35 ` Alexander Turenko
2020-03-27 8:39 ` Konstantin Osipov
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 4/6] error: Add extended error transfer format Leonid Vasiliev
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 5/6] error: Add test for extended error Leonid Vasiliev
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 6/6] error: Transmit an error through IPROTO_OK as object Leonid Vasiliev
2020-03-27 23:11 ` [Tarantool-patches] [PATCH 0/6] Extending error functionality lvasiliev
2020-03-28 13:54 ` Alexander Turenko
2020-03-30 10:48 ` lvasiliev
2020-04-01 15:35 ` Alexander Turenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox