* [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber
@ 2019-09-02 14:51 Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-02 14:51 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, kostja, Kirill Shcherbatov
Support stacked diagnostics for Tarantool allows to accumulate all
occurred errors during processing a request. This allows to better
understand what has happened and handle errors correspondingly.
This patch introduces core methods diag_add(), diag_svp(),
diag_rollback_to_svp() that allows you to produce a better
diagnostics information for a user.
There is an other new Lua endpoint
box.error.prev() that allows you to get a reason for a given
error object.
Changes in v3:
- fixed Kostya's comments
- implemented iproto support for stacked diagnostics
Changes in v2:
- new API - user shouldn't have an ability to modify an existent
error object that he didn't constructed.
- a new detailed unit-test that performs a good coverage for
many complicated reference-counting scenarios.
Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-1148-stacked-errors
Issue: https://github.com/tarantool/tarantool/issues/1148
Kirill Shcherbatov (6):
box: rfc for stacked diagnostic area in Tarantool
box: rename diag_add_error to diag_set_error
box: introduce stacked diagnostic area
box: extend box.error.new({}) API
iproto: refactor error encoding with mpstream
iproto: support stacked diagnostics for errors
src/box/error.h | 5 +-
src/box/iproto_constants.h | 1 +
src/lib/core/diag.h | 84 +++++++++++-
src/lib/core/exception.h | 2 +-
src/lua/error.h | 3 +
src/box/key_list.c | 16 +--
src/box/lua/call.c | 6 +-
src/box/sql/vdbe.c | 2 +-
src/box/vy_scheduler.c | 6 +-
src/box/xrow.c | 198 +++++++++++++++++++++++-----
src/lib/core/diag.c | 1 +
src/lua/error.c | 2 +-
src/lua/utils.c | 2 +-
test/app-tap/module_api.c | 3 +-
test/box/function1.c | 29 ++--
test/box/reload1.c | 20 +--
test/box/reload2.c | 9 +-
test/box/tuple_bench.c | 10 +-
doc/rfc/1148-stacked-diagnostics.md | 180 +++++++++++++++++++++++++
src/box/applier.cc | 2 +-
src/box/error.cc | 7 +-
src/box/lua/error.cc | 35 ++++-
src/box/lua/net_box.lua | 24 +++-
src/box/relay.cc | 4 +-
src/lib/core/exception.cc | 2 +-
src/lua/error.lua | 12 ++
test/app/fiber.result | 7 +-
test/box-py/iproto.result | 6 +-
test/box-py/iproto.test.py | 6 +-
test/box/misc.result | 159 +++++++++++++++++++++-
test/box/misc.test.lua | 49 +++++++
test/engine/func_index.result | 52 ++++++--
test/engine/func_index.test.lua | 7 +
test/unit/CMakeLists.txt | 2 +-
test/unit/fiber.cc | 157 ++++++++++++++++++++++
test/unit/fiber.result | 60 +++++++++
36 files changed, 1045 insertions(+), 125 deletions(-)
create mode 100644 doc/rfc/1148-stacked-diagnostics.md
--
2.22.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
@ 2019-09-02 14:51 ` Kirill Shcherbatov
2019-09-09 8:13 ` [tarantool-patches] " Kirill Shcherbatov
2019-09-09 19:44 ` Vladislav Shpilevoy
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 2/6] box: rename diag_add_error to diag_set_error Kirill Shcherbatov
` (4 subsequent siblings)
5 siblings, 2 replies; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-02 14:51 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, kostja, Kirill Shcherbatov
Part of #1148
---
doc/rfc/1148-stacked-diagnostics.md | 180 ++++++++++++++++++++++++++++
1 file changed, 180 insertions(+)
create mode 100644 doc/rfc/1148-stacked-diagnostics.md
diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
new file mode 100644
index 000000000..3e455ec97
--- /dev/null
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -0,0 +1,180 @@
+# Stacked Diagnostics in Tarantool
+
+* **Status**: In progress
+* **Start date**: 30-07-2019
+* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org, Konstantin Osipov @kostja kostja@tarantool.org, Georgy Kirichenko @GeorgyKirichenko georgy@tarantool.org, @Totktonada Alexander Turenko alexander.turenko@tarantool.org,Vladislav Shpilevoy @Gerold103 v.shpilevoy@tarantool.org
+* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
+
+## Summary
+Support stacked diagnostics for Tarantool allows to accumulate all occurred errors during processing a request. This allows to better understand what has happened and handle errors
+correspondingly.
+
+## Background and motivation
+
+Tarantool statements must produce diagnostic information that populates the diagnostics area. This is a Standard SQL requirement and other vendors and languages also have such feature.
+Diagnostics area stack must contain a diagnostics area for each nested execution context.
+
+### Current Tarantool's error diagnostics
+Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
+The last error is exported with `box.error.last()` endpoint.
+
+In total there are few error classes in Tarantool.
+```
+ClientError
+ | LoggedError
+ | AccessDeniedError
+ | UnsupportedIndexFeature
+
+XlogError
+ | XlogGapError
+SystemError
+ | SocketError
+ | OutOfMemory
+ | TimedOut
+
+ChannelIsClosed
+FiberIsCancelled
+LuajitError
+IllegalParams
+CollationError
+SwimError
+CryptoError
+```
+
+All ClientError codes are exported with their error codes in box.error folder by their NAMEs.
+
+You may use box.error.new endpoint to create a new error instance of predefined type:
+```
+tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
+tarantool> t:unpack()
+---
+- type: ClientError
+ code: 9
+ message: 'Failed to create space ''myspace'': just I can''t'
+ trace:
+ - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
+ line: 1
+```
+
+User is allowed to define own errors with any code (including system errors) with
+```
+box.error.new({code = user_code, reason = user_error_msg})
+```
+
+Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()` methods and uniform `.type`, `.message` and `.trace` fields.
+
+
+## Proposal
+In some cases a diagnostic information must be more complicated.
+For example, when persistent Lua function referenced by functional index has a bug in it's definition, Lua handler sets an diag message. Then functional index extractor code setups an own, more specialized error.
+
+We need to introduce instruments to deal with it both in C and Lua API. Let's overview them one-by-one:
+
+### C API
+The existent `diag_set()` method should be kept as is: it should replace the last error in diagnostic area with a new one.
+
+Let's introduce a new method `diag_add()` that keeps an existent error message in diagnostic area (if any) and sets it as a reason error for a recently-constructed error object.
+
+We also need a way to reset last errors set. Let's introduce `diag_svp(diag) -> SVP`,
+`diag_rollback_to_svp(diag, SVP)`. The implementation proposal for SVP structure is given below:
+
+```
+ truncate
+ xxxxxxxxxxxxxxxxxxxxxxxx SVP
+DIAG: +-------+ |
+ | err | |
+ +-------+----->+-------+ here
+ third prev | err |
+ +-------+------->+-------+
+ second prev | err |
+ +-------+
+ first
+```
+
+The diag_structure already has `struct error *next` pointer;
+
+Let's introduce a similar pointer in an error structure to organize a reason list:
+```
+struct error {
+ ...
+ struct error *prev;
+};
+```
+
+To implement a SVP object, we may return a last-set error pointer (diag::last):
+```
+diag_set(diag, FIRST)
+struct error *SVP = diag_svp(diag)
+ /**********************************\
+ * return diag->last *
+ \**********************************/
+
+diag_add(diag, SECOND)
+diag_add(diag, THIRD)
+
+diag_rollback_to_svp(diag, svp)
+ /***********************************\
+ * struct error *tmp = diag->last; *
+ * while (tmp != svp) { *
+ * diag->last = tmp->prev; *
+ * error_unref(tmp); *
+ * tmp = diag->last; *
+ * } *
+ \***********************************/
+```
+
+### Lua API
+Tarantool returns a last-set (diag::last) error object as `cdata` from central diagnostic
+area to Lua in case of error. User shouldn't have an ability to modify it
+(because he actually needn't and because it is dangerous - he doesn't own this object),
+but it need an ability to inspect a collected diagnostics information.
+
+Let's extend the `box.error` API with a couple of new functions:
+1) new parameters for box.error.new():
+ - filename - the name of the file where this error has been initially raised
+ - line - the number of the line where this error has been initially raised
+ - prev - the 'reason' parent error object. A new error would take a reference on it.
+1) `prev` that allows to get a reason of given error:
+
+```
+-- Return a reason error object for given error
+-- (when exists, nil otherwise)
+box.error.prev(error) == error.prev
+```
+### Binary protocol
+Currently errors are sent as `(IPROTO_ERROR | errcode)` messages with an error's message string payload.
+
+We must to design a backward-compatible error transfer specification. Imagine a net.box old-new Tarantool connection, same as existing connector: they mustn't broken.
+
+Let's extend existent binary protocol with a new key IPROTO_ERROR_V2:
+{
+ // backward compatibility
+ IPROTO_ERROR: "the most recent error message",
+ // modern error message
+ IPROTO_ERROR_V2: {
+ {
+ // the most recent error object
+ "code": error_code_number,
+ "reason": error_reason_string,
+ "file": file_name_string,
+ "line": line_number,
+ },
+ ...
+ {
+ // the oldest (reason) error object
+ },
+ }
+}
+
+### SQL Warnings
+SQL Standard defines ["WARNINGS"](https://docs.microsoft.com/en-us/sql/t-sql/statements/set-ansi-warnings-transact-sql?view=sql-server-2017) term:
+- When set to ON, if null values appear in aggregate functions, such as SUM, AVG, MAX, MIN, STDEV, STDEVP, VAR, VARP, or COUNT, a warning message is generated. When set to OFF, no warning is issued.
+- When set to ON, the divide-by-zero and arithmetic overflow errors cause the statement to be rolled back and an error message is generated. When set to OFF, the divide-by-zero and arithmetic overflow errors cause null values to be returned. The behavior in which a divide-by-zero or arithmetic overflow error causes null values to be returned occurs if an INSERT or UPDATE is tried on a character, Unicode, or binary column in which the length of a new value exceeds the maximum size of the column. If SET ANSI_WARNINGS is ON, the INSERT or UPDATE is canceled as specified by the ISO standard. Trailing blanks are ignored for character columns and trailing nulls are ignored for binary columns. When OFF, data is truncated to the size of the column and the statement succeeds.
+
+According to the current convention, all Tarantool's function use "return not null error code on error" convention. Iff the global diagnostic area is valid (actually, Unix errno uses the same semantics). So we need an additional `Parser`/`VDBE` marker to indicate that the warning diagnostics
+is set. Say, in addition to `bool is_aborted` field we may introduce and additional field `bool is_warning_set`.
+
+To avoid a mess between SQL warnings and real errors let's better introduce an
+"diagnostics"-semantics area in Parser/VDBE classes where all warnings are
+organized in a list. Internal warning representation needn't follow error
+specification and may use other representation when it is reasonable.
--
2.22.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v3 2/6] box: rename diag_add_error to diag_set_error
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
@ 2019-09-02 14:51 ` Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 3/6] box: introduce stacked diagnostic area Kirill Shcherbatov
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-02 14:51 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, kostja, Kirill Shcherbatov
Renamed function diag_add_error to diag_set_error because it
actually replaces an error object in diagnostic area with a new
one and this name is not representative. Moreover, we are going
to introduce a new diag_add_error that extends error object in
diagnostic area in following patches.
Needed for #1148
---
src/lib/core/diag.h | 6 +++---
src/lib/core/exception.h | 2 +-
src/box/vy_scheduler.c | 6 +++---
src/lua/utils.c | 2 +-
src/box/applier.cc | 2 +-
src/box/error.cc | 2 +-
src/box/relay.cc | 4 ++--
src/lib/core/exception.cc | 2 +-
8 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index fd3831e66..02a67269f 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -162,12 +162,12 @@ diag_clear(struct diag *diag)
}
/**
- * Add a new error to the diagnostics area
+ * Set a new error to the diagnostics area, replacing existent.
* \param diag diagnostics area
* \param e error to add
*/
static inline void
-diag_add_error(struct diag *diag, struct error *e)
+diag_set_error(struct diag *diag, struct error *e)
{
assert(e != NULL);
error_ref(e);
@@ -269,7 +269,7 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
say_debug("%s at %s:%i", #class, __FILE__, __LINE__); \
struct error *e; \
e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__); \
- diag_add_error(diag_get(), e); \
+ diag_set_error(diag_get(), e); \
/* Restore the errno which might have been reset. */ \
errno = save_errno; \
} while (0)
diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
index a29281427..e1a45b3fb 100644
--- a/src/lib/core/exception.h
+++ b/src/lib/core/exception.h
@@ -182,7 +182,7 @@ exception_init();
#define tnt_error(class, ...) ({ \
say_debug("%s at %s:%i", #class, __FILE__, __LINE__); \
class *e = new class(__FILE__, __LINE__, ##__VA_ARGS__); \
- diag_add_error(diag_get(), e); \
+ diag_set_error(diag_get(), e); \
e; \
})
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index ee361c31f..31d144cf3 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -618,7 +618,7 @@ vy_scheduler_dump(struct vy_scheduler *scheduler)
if (scheduler->is_throttled) {
/* Dump error occurred. */
struct error *e = diag_last_error(&scheduler->diag);
- diag_add_error(diag_get(), e);
+ diag_set_error(diag_get(), e);
return -1;
}
fiber_cond_wait(&scheduler->dump_cond);
@@ -692,7 +692,7 @@ vy_scheduler_begin_checkpoint(struct vy_scheduler *scheduler)
*/
if (scheduler->is_throttled) {
struct error *e = diag_last_error(&scheduler->diag);
- diag_add_error(diag_get(), e);
+ diag_set_error(diag_get(), e);
say_error("cannot checkpoint vinyl, "
"scheduler is throttled with: %s", e->errmsg);
return -1;
@@ -728,7 +728,7 @@ vy_scheduler_wait_checkpoint(struct vy_scheduler *scheduler)
if (scheduler->is_throttled) {
/* A dump error occurred, abort checkpoint. */
struct error *e = diag_last_error(&scheduler->diag);
- diag_add_error(diag_get(), e);
+ diag_set_error(diag_get(), e);
say_error("vinyl checkpoint failed: %s", e->errmsg);
return -1;
}
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 75efe0ed2..22b0f0424 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1004,7 +1004,7 @@ luaT_toerror(lua_State *L)
struct error *e = luaL_iserror(L, -1);
if (e != NULL) {
/* Re-throw original error */
- diag_add_error(&fiber()->diag, e);
+ diag_set_error(&fiber()->diag, e);
} else {
/* Convert Lua error to a Tarantool exception. */
diag_set(LuajitError, luaT_tolstring(L, -1, NULL));
diff --git a/src/box/applier.cc b/src/box/applier.cc
index 6239fcfd3..565db953c 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -763,7 +763,7 @@ applier_on_rollback(struct trigger *trigger, void *event)
struct applier *applier = (struct applier *)trigger->data;
/* Setup a shared error. */
if (!diag_is_empty(&replicaset.applier.diag)) {
- diag_add_error(&applier->diag,
+ diag_set_error(&applier->diag,
diag_last_error(&replicaset.applier.diag));
}
/* Stop the applier fiber. */
diff --git a/src/box/error.cc b/src/box/error.cc
index 47dce3305..7dfe1b3ee 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -82,7 +82,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
error_vformat_msg(e, fmt, ap);
va_end(ap);
}
- diag_add_error(&fiber()->diag, e);
+ diag_set_error(&fiber()->diag, e);
return -1;
}
diff --git a/src/box/relay.cc b/src/box/relay.cc
index a19abf6a9..1f6cb28b7 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -449,7 +449,7 @@ relay_set_error(struct relay *relay, struct error *e)
{
/* Don't override existing error. */
if (diag_is_empty(&relay->diag))
- diag_add_error(&relay->diag, e);
+ diag_set_error(&relay->diag, e);
}
static void
@@ -623,7 +623,7 @@ relay_subscribe_f(va_list ap)
* Don't clear the error for status reporting.
*/
assert(!diag_is_empty(&relay->diag));
- diag_add_error(diag_get(), diag_last_error(&relay->diag));
+ diag_set_error(diag_get(), diag_last_error(&relay->diag));
diag_log();
say_crit("exiting the relay loop");
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index a6999af43..55ee8cd40 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -99,7 +99,7 @@ Exception::operator new(size_t size)
void *buf = malloc(size);
if (buf != NULL)
return buf;
- diag_add_error(diag_get(), &out_of_memory);
+ diag_set_error(diag_get(), &out_of_memory);
throw &out_of_memory;
}
--
2.22.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v3 3/6] box: introduce stacked diagnostic area
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 2/6] box: rename diag_add_error to diag_set_error Kirill Shcherbatov
@ 2019-09-02 14:51 ` Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 4/6] box: extend box.error.new({}) API Kirill Shcherbatov
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-02 14:51 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, kostja, Kirill Shcherbatov
Part of #1148
@TarantoolBot document
Title: Stacked Diagnostics for Tarantool
Tarantool statements must produce diagnostic information that
populates the diagnostics area. Diagnostics area stack must
contain a diagnostics area for each nested execution context.
Tarantool used to have the only diag_set() mechanism to set a
diagnostic error. In some cases a diagnostic information must be
more complicated. A new method diag_add() allows to extend
global diagnostics with a new error: the previous set becomes
a reason for a recently-constructed error object. This commit
also introduce a savepoint-semantics mechanism to remove errors
when they are redundant. Thus,
diag_set(diag, FIRST)
struct error *SVP = diag_svp(diag)
diag_add(diag, SECOND)
diag_add(diag, THIRD)
diag_rollback_to_svp(diag, svp) // only the FIRST error is set
To work with errors having a reason efficiently, box.error
endpoint was extended with a new method prev that returns a
reason error object for a given error, when exists or nil
otherwise:
err = box.error.last()
err:unpack()
reason = box.error.prev(err)
assert(err.prev == reason)
reason:unpack()
---
src/lib/core/diag.h | 80 +++++++++++++++-
src/lua/error.h | 3 +
src/box/key_list.c | 16 ++--
src/box/lua/call.c | 6 +-
src/lib/core/diag.c | 1 +
src/lua/error.c | 2 +-
src/box/lua/error.cc | 20 ++++
src/lua/error.lua | 12 +++
test/app/fiber.result | 7 +-
test/box/misc.result | 7 +-
test/engine/func_index.result | 52 +++++++++--
test/engine/func_index.test.lua | 7 ++
test/unit/CMakeLists.txt | 2 +-
test/unit/fiber.cc | 157 ++++++++++++++++++++++++++++++++
test/unit/fiber.result | 60 ++++++++++++
15 files changed, 403 insertions(+), 29 deletions(-)
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 02a67269f..91596b2e6 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -78,6 +78,8 @@ struct error {
char file[DIAG_FILENAME_MAX];
/* Error description. */
char errmsg[DIAG_ERRMSG_MAX];
+ /* A pointer to the reason error. */
+ struct error *prev;
};
static inline void
@@ -90,9 +92,13 @@ static inline void
error_unref(struct error *e)
{
assert(e->refs > 0);
- --e->refs;
- if (e->refs == 0)
+ while (--e->refs == 0) {
+ struct error *prev = e->prev;
e->destroy(e);
+ if (prev == NULL)
+ break;
+ e = prev;
+ }
}
NORETURN static inline void
@@ -175,6 +181,26 @@ diag_set_error(struct diag *diag, struct error *e)
diag->last = e;
}
+/**
+ * Add a new error to the diagnostics area: the previous error
+ * becomes a reason of a current.
+ * \param diag diagnostics area
+ * \param e error to add
+ */
+static inline void
+diag_add_error(struct diag *diag, struct error *e)
+{
+ assert(e != NULL);
+ error_ref(e);
+ /*
+ * Nominally e takes a reason's reference while diag
+ * releases it's reference because it holds e now
+ * instead. I.e. reason->refs kept unchanged.
+ */
+ e->prev = diag->last;
+ diag->last = e;
+}
+
/**
* Move all errors from \a from to \a to.
* \param from source
@@ -212,6 +238,45 @@ diag_last_error(struct diag *diag)
return diag->last;
}
+/**
+ * Get a diagnostic savepoint: a marker that allows to reset all
+ * errors set after that moment.
+ */
+static inline struct error *
+diag_svp(struct diag *diag)
+{
+ return diag_last_error(diag);
+}
+
+/**
+ * Remove all errors set in a given diagnostics area after a
+ * given savepoint.
+ *
+ * Operation removes reason for the error
+ * preceding the savepoint and releases a diagnostic area's
+ * reference on the most recent error (diag::last for the
+ * rollback beginning). This means that if user code have a
+ * pointer and have a reference to an error object from the
+ * rollback zone, this pointer and the following "reason" error
+ * objects are a valid error list.
+ */
+static inline void
+diag_rollback_to_svp(struct diag *diag, struct error *svp)
+{
+ struct error *begin = diag->last, *err = NULL;
+ while (diag->last != svp) {
+ err = diag->last;
+ diag->last = diag->last->prev;
+ }
+ if (diag->last != begin) {
+ assert(err != NULL && err->prev == svp);
+ err->prev = NULL;
+ error_unref(begin);
+ err->prev = svp;
+ error_ref(svp);
+ }
+}
+
struct diag *
diag_get();
@@ -274,6 +339,17 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
errno = save_errno; \
} while (0)
+#define diag_add(class, ...) do { \
+ /* Preserve the original errno. */ \
+ int save_errno = errno; \
+ say_debug("%s at %s:%i", #class, __FILE__, __LINE__); \
+ struct error *e; \
+ e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__); \
+ diag_add_error(diag_get(), e); \
+ /* Restore the errno which might have been reset. */ \
+ errno = save_errno; \
+} while (0)
+
#if defined(__cplusplus)
} /* extern "C" */
#endif /* defined(__cplusplus) */
diff --git a/src/lua/error.h b/src/lua/error.h
index 64fa5eba3..16cdaf7fe 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/box/key_list.c b/src/box/key_list.c
index e130d1c8c..c3de262cc 100644
--- a/src/box/key_list.c
+++ b/src/box/key_list.c
@@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
if (rc != 0) {
/* Can't evaluate function. */
struct space *space = space_by_id(index_def->space_id);
- diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
- space ? space_name(space) : "",
- diag_last_error(diag_get())->errmsg);
+ diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+ space != NULL ? space_name(space) : "",
+ "can't evaluate function");
return -1;
}
uint32_t key_data_sz;
@@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
if (key_data == NULL) {
struct space *space = space_by_id(index_def->space_id);
/* Can't get a result returned by function . */
- diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
- space ? space_name(space) : "",
- diag_last_error(diag_get())->errmsg);
+ diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+ space != NULL ? space_name(space) : "",
+ "can't get a value returned by function");
return -1;
}
@@ -170,9 +170,9 @@ key_list_iterator_next(struct key_list_iterator *it, const char **value)
* The key doesn't follow functional index key
* definition.
*/
- diag_set(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
+ diag_add(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
space ? space_name(space) : "",
- diag_last_error(diag_get())->errmsg);
+ "key does not follow functional index definition");
return -1;
}
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 631003c84..0d510c4e1 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -683,9 +683,9 @@ func_persistent_lua_load(struct func_lua *func)
if (func->base.def->is_sandboxed) {
if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports,
nelem(default_sandbox_exports)) != 0) {
- diag_set(ClientError, ER_LOAD_FUNCTION,
- func->base.def->name,
- diag_last_error(diag_get())->errmsg);
+ diag_add(ClientError, ER_LOAD_FUNCTION,
+ func->base.def->name,
+ "can't prepare a Lua sandbox");
goto end;
}
} else {
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index 248277e74..6ad3378a5 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -52,6 +52,7 @@ error_create(struct error *e,
e->line = 0;
}
e->errmsg[0] = '\0';
+ e->prev = NULL;
}
struct diag *
diff --git a/src/lua/error.c b/src/lua/error.c
index d82e78dc4..18a990a88 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -53,7 +53,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);
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 230d51dec..5a2f5fc7d 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -144,6 +144,22 @@ luaT_error_new(lua_State *L)
return luaT_error_last(L);
}
+static int
+luaT_error_prev(lua_State *L)
+{
+ if (lua_gettop(L) == 0)
+ return luaL_error(L, "Usage: box.error.prev(error)");
+ struct error *e = luaL_checkerror(L, 1);
+ if (e == NULL)
+ return luaT_error(L);
+
+ if (e->prev != NULL)
+ luaT_pusherror(L, e->prev);
+ else
+ lua_pushnil(L);
+ return 1;
+}
+
static int
luaT_error_clear(lua_State *L)
{
@@ -250,6 +266,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_prev);
+ lua_setfield(L, -2, "prev");
+ }
lua_setfield(L, -2, "__index");
}
lua_setmetatable(L, -2);
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 28fc0377d..0f76f6363 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -23,6 +23,8 @@ struct error {
char _file[DIAG_FILENAME_MAX];
/* Error description. */
char _errmsg[DIAG_ERRMSG_MAX];
+ /* A pointer to the reason error. */
+ struct error *_prev;
};
char *
@@ -86,10 +88,20 @@ local function error_trace(err)
}
end
+local function error_refs(err)
+ return err._refs
+end
+
+local function error_prev(err)
+ return box.error.prev(err)
+end
+
local error_fields = {
["type"] = error_type;
["message"] = error_message;
["trace"] = error_trace;
+ ["refs"] = error_refs;
+ ["prev"] = error_prev;
}
local function error_unpack(err)
diff --git a/test/app/fiber.result b/test/app/fiber.result
index 94e690f6c..a10d43ba9 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1038,12 +1038,13 @@ st;
...
e:unpack();
---
-- type: ClientError
- code: 1
- message: Illegal parameters, oh my
+- code: 1
trace:
- file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
line: 1
+ type: ClientError
+ message: Illegal parameters, oh my
+ refs: 2
...
flag = false;
---
diff --git a/test/box/misc.result b/test/box/misc.result
index c46c5a9d6..555075a6c 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -125,12 +125,13 @@ e
...
e:unpack()
---
-- type: ClientError
- code: 1
- message: Illegal parameters, bla bla
+- code: 1
trace:
- file: '[C]'
line: 4294967295
+ type: ClientError
+ message: Illegal parameters, bla bla
+ refs: 4
...
e.type
---
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index bb4200f7a..e4b3db7a6 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -5,6 +5,10 @@ test_run = require('test_run').new()
engine = test_run:get_cfg('engine')
| ---
| ...
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+ | ---
+ | - true
+ | ...
--
-- gh-1260: Func index.
@@ -158,8 +162,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn1.id, parts = {{1, 'un
s:insert({1})
| ---
| - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- | ''withdata'': Supplied key type of part 0 does not match index part type: expected
- | unsigned'
+ | ''withdata'': key does not follow functional index definition'
| ...
idx:drop()
| ---
@@ -197,8 +200,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'un
s:insert({1})
| ---
| - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- | ''withdata'': Supplied key type of part 0 does not match index part type: expected
- | unsigned'
+ | ''withdata'': key does not follow functional index definition'
| ...
idx:drop()
| ---
@@ -217,8 +219,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn4.id, parts = {{1, 'un
s:insert({1})
| ---
| - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- | ''withdata'': Supplied key type of part 0 does not match index part type: expected
- | unsigned'
+ | ''withdata'': key does not follow functional index definition'
| ...
idx:drop()
| ---
@@ -264,8 +265,43 @@ idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'stri
s:insert({1})
| ---
| - error: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
- | [string "return function(tuple) local ..."]:1: attempt to call
- | global ''require'' (a nil value)'
+ | can''t evaluate function'
+ | ...
+e = box.error.last()
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 198
+ | trace:
+ | - file: <filename>
+ | line: 68
+ | type: ClientError
+ | prev: '[string "return function(tuple) local ..."]:1: attempt to
+ | call global ''require'' (a nil value)'
+ | message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ | can''t evaluate function'
+ | refs: 3
+ | ...
+e = box.error.prev(e)
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - type: LuajitError
+ | refs: 3
+ | message: '[string "return function(tuple) local ..."]:1: attempt
+ | to call global ''require'' (a nil value)'
+ | trace:
+ | - file: <filename>
+ | line: 1010
+ | ...
+e = box.error.prev(e)
+ | ---
+ | ...
+e == nil
+ | ---
+ | - true
| ...
idx:drop()
| ---
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index f31162c97..ccbc9822d 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -1,5 +1,6 @@
test_run = require('test_run').new()
engine = test_run:get_cfg('engine')
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
--
-- gh-1260: Func index.
@@ -99,6 +100,12 @@ test_run:cmd("setopt delimiter ''");
box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true})
idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
s:insert({1})
+e = box.error.last()
+e:unpack()
+e = box.error.prev(e)
+e:unpack()
+e = box.error.prev(e)
+e == nil
idx:drop()
-- Remove old persistent functions
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 4a57597e9..6dfe1f6d9 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -72,7 +72,7 @@ target_link_libraries(decimal.test core unit)
add_executable(fiber.test fiber.cc)
set_source_files_properties(fiber.cc PROPERTIES COMPILE_FLAGS -O0)
-target_link_libraries(fiber.test core unit)
+target_link_libraries(fiber.test core box unit)
if (NOT ENABLE_GCOV)
# This test is known to be broken with GCOV
diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
index 91f7d43f9..2bec8bdf8 100644
--- a/test/unit/fiber.cc
+++ b/test/unit/fiber.cc
@@ -1,3 +1,6 @@
+#include "box/error.h"
+#include "diag.h"
+#include "errcode.h"
#include "memory.h"
#include "fiber.h"
#include "unit.h"
@@ -181,12 +184,166 @@ fiber_name_test()
footer();
}
+void
+diag_test()
+{
+ header();
+ plan(28);
+
+ note("constract e1 in global diag, share with local diag");
+ diag_set(ClientError, ER_PROC_LUA, "runtime error");
+ struct diag local_diag;
+ diag_create(&local_diag);
+ /* Copy a last error to the local diagnostics area. */
+ diag_add_error(&local_diag, diag_last_error(diag_get()));
+
+ struct error *e1 = diag_last_error(&local_diag);
+ is(e1, diag_last_error(diag_get()),
+ "e1 is an error shared between local and global diag");
+ is(e1->refs, 2, "e1::refs: global+local diag");
+
+ note("append e2 to global diag, usr error_ref(e2)");
+ diag_add(ClientError, ER_LOAD_FUNCTION, "test", "internal error");
+ struct error *e2 = diag_last_error(diag_get());
+ error_ref(e2);
+
+ is(e2->prev, e1, "e2::prev == e1");
+ is(e1->prev, NULL, "e1::prev == NULL");
+ is(e1->refs, 2, "e1::refs: e2 + global diag");
+ is(e2->refs, 2, "e2::refs: usr + global diag");
+
+ note("diag_clean global diag");
+ diag_clear(diag_get());
+ is(e1->refs, 2, "e1::refs: e2 + local");
+ is(e2->refs, 1, "e2::refs: usr");
+ note("error_unref(e2) -> e2 destruction");
+ error_unref(e2);
+ e2 = NULL;
+ is(e1->refs, 1, "e1::refs: local diag");
+
+ /* Test rollback to SVP. */
+ note("diag_move(from = local, to = global): move e1 to global");
+ diag_move(&local_diag, diag_get());
+ is(diag_is_empty(&local_diag), true, "local diag is empty");
+ is(diag_is_empty(diag_get()), false, "global diag is not empty");
+ is(diag_last_error(diag_get()), e1, "global diag::last == e1");
+
+ note("svp = diag_svp(global), i.e. 'diag_last(global) = e1' state");
+ struct error *svp = diag_svp(diag_get());
+ fail_if(svp != e1);
+ fail_if(diag_last_error(diag_get()) != e1);
+ fail_if(e1->prev != NULL);
+ note("append e3, e4 to global diag");
+ note("usr error_ref(e1), error_ref(e3), error_ref(e4)");
+ diag_add(ClientError, ER_LOAD_FUNCTION, "test", "internal error");
+ struct error *e3 = diag_last_error(diag_get());
+ error_ref(e3);
+ diag_add(ClientError, ER_FUNC_INDEX_FUNC, "func_idx", "space",
+ "everything is bad");
+ struct error *e4 = diag_last_error(diag_get());
+ error_ref(e4);
+ is(e1->refs, 1, "e1::refs: e3");
+ is(e3->refs, 2, "e3::refs: usr + e4");
+ is(e4->refs, 2, "e4::refs: usr + global diag");
+ is(e4->prev, e3, "e4::prev == e3");
+ is(e3->prev, e1, "e3::prev == e1");
+ note("diag_rollback_to_svp(global, svp)");
+ /*
+ * Before rollback there is a sequence
+ * DIAG[e4]->e3->e1->NULL;
+ * After rollback there would be DIAG[e1]->NULL and
+ * a sequence e4->e3->e1->NULL.
+ */
+ diag_rollback_to_svp(diag_get(), svp);
+ is(e1->refs, 2, "e1::refs: e3 + global diag %d/%d", e1->refs, 2);
+ is(e3->refs, 2, "e3::refs: usr + e4");
+ is(e4->refs, 1, "e4::refs: usr");
+ is(diag_last_error(diag_get()), e1, "diag_last(global) = e1");
+ /* Rollback doesn't change error objects itself. */
+ is(e4->prev, e3, "e4::prev == e3");
+ is(e3->prev, e1, "e3::prev == e1");
+ error_unref(e4);
+ e4 = NULL;
+ is(e3->refs, 1, "e3::refs: usr");
+ error_unref(e3);
+ e3 = NULL;
+
+ note("ensure that sequential rollback is no-op");
+ diag_rollback_to_svp(diag_get(), svp);
+ is(e1->refs, 1, "e1::refs: global diag");
+
+ diag_clear(diag_get());
+ /*
+ * usr ref SVP
+ * DEL! | |
+ * DIAG[#5] -> #4 -> DIAG'[#3] -> #2 -> #1
+ *
+ * 1) diag_rollback_to_svp
+ * del <-----------<------>
+ */
+ note("test partial list destruction on rollback");
+ diag_add(ClientError, ER_PROC_LUA, "#1");
+ struct error *er1 = diag_last_error(diag_get());
+ diag_add(ClientError, ER_PROC_LUA, "#2");
+ svp = diag_svp(diag_get());
+ diag_add(ClientError, ER_PROC_LUA, "#3");
+ struct error *er3 = diag_last_error(diag_get());
+ diag_add(ClientError, ER_PROC_LUA, "#4");
+ struct error *er4 = diag_last_error(diag_get());
+ error_ref(er4);
+ diag_add(ClientError, ER_PROC_LUA, "#5");
+ is(er4->refs, 2, "er4:refs: usr + er5 %d/%d", er4->refs, 2);
+
+ diag_rollback_to_svp(diag_get(), svp);
+ note("rollback to svp(er2) -> e5:refs == 0, destruction");
+ is(er4->prev, er3, "er4->prev == er3");
+ is(er3->refs, 1, "er3:refs: er4");
+ is(er3->prev, svp, "er3->prev == svp");
+ is(svp->refs, 2, "svp->refs: global diag + er3");
+ is(svp->prev, er1, "svp->prev == er1");
+ is(er1->refs, 1, "er1->refs: err2");
+
+ /*
+ * usr ref SVP
+ * | |
+ * #4 -> #3 -> DIAG'[#2] -> #1
+ * |
+ * DIAG[#7] -> #6 -/
+ * |
+ * usr ref
+ */
+ note("multiple error sequences after rollback");
+ diag_add(ClientError, ER_PROC_LUA, "#6");
+ diag_add(ClientError, ER_PROC_LUA, "#7");
+ struct error *er7 = diag_last_error(diag_get());
+ error_ref(er7);
+ is(er4->refs, 1, "er4->refs: usr");
+ is(er7->refs, 2, "er7->refs: global diag + usr");
+ is(svp->refs, 2, "svp->refs: er3 + er6");
+ is(svp->prev->refs, 1, "svp->prev->refs: svp");
+ diag_rollback_to_svp(diag_get(), svp);
+ is(er4->refs, 1, "er4->refs: usr");
+ is(er7->refs, 1, "er7->refs: usr");
+ is(svp->refs, 3, "svp->refs: global diag + er3 + er6");
+ is(svp->prev->refs, 1, "svp->prev->refs: svp");
+ diag_clear(diag_get());
+ is(svp->refs, 2, "svp->refs: er3 + er6");
+ is(svp->prev->refs, 1, "svp->prev->refs: svp");
+ error_unref(er4);
+ is(svp->refs, 1, "svp->refs: er6");
+ is(svp->prev->refs, 1, "svp->prev->refs: svp");
+ error_unref(er7);
+
+ footer();
+}
+
static int
main_f(va_list ap)
{
fiber_name_test();
fiber_join_test();
fiber_stack_test();
+ diag_test();
ev_break(loop(), EVBREAK_ALL);
return 0;
}
diff --git a/test/unit/fiber.result b/test/unit/fiber.result
index 7c9f85dcd..58e67b4b9 100644
--- a/test/unit/fiber.result
+++ b/test/unit/fiber.result
@@ -17,3 +17,63 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc
# normal-stack fiber not crashed
# big-stack fiber not crashed
*** fiber_stack_test: done ***
+ *** diag_test ***
+1..28
+# constract e1 in global diag, share with local diag
+ok 1 - e1 is an error shared between local and global diag
+ok 2 - e1::refs: global+local diag
+# append e2 to global diag, usr error_ref(e2)
+ok 3 - e2::prev == e1
+ok 4 - e1::prev == NULL
+ok 5 - e1::refs: e2 + global diag
+ok 6 - e2::refs: usr + global diag
+# diag_clean global diag
+ok 7 - e1::refs: e2 + local
+ok 8 - e2::refs: usr
+# error_unref(e2) -> e2 destruction
+ok 9 - e1::refs: local diag
+# diag_move(from = local, to = global): move e1 to global
+ok 10 - local diag is empty
+ok 11 - global diag is not empty
+ok 12 - global diag::last == e1
+# svp = diag_svp(global), i.e. 'diag_last(global) = e1' state
+# append e3, e4 to global diag
+# usr error_ref(e1), error_ref(e3), error_ref(e4)
+ok 13 - e1::refs: e3
+ok 14 - e3::refs: usr + e4
+ok 15 - e4::refs: usr + global diag
+ok 16 - e4::prev == e3
+ok 17 - e3::prev == e1
+# diag_rollback_to_svp(global, svp)
+ok 18 - e1::refs: e3 + global diag 2/2
+ok 19 - e3::refs: usr + e4
+ok 20 - e4::refs: usr
+ok 21 - diag_last(global) = e1
+ok 22 - e4::prev == e3
+ok 23 - e3::prev == e1
+ok 24 - e3::refs: usr
+# ensure that sequential rollback is no-op
+ok 25 - e1::refs: global diag
+# test partial list destruction on rollback
+ok 26 - er4:refs: usr + er5 2/2
+# rollback to svp(er2) -> e5:refs == 0, destruction
+ok 27 - er4->prev == er3
+ok 28 - er3:refs: er4
+ok 29 - er3->prev == svp
+ok 30 - svp->refs: global diag + er3
+ok 31 - svp->prev == er1
+ok 32 - er1->refs: err2
+# multiple error sequences after rollback
+ok 33 - er4->refs: usr
+ok 34 - er7->refs: global diag + usr
+ok 35 - svp->refs: er3 + er6
+ok 36 - svp->prev->refs: svp
+ok 37 - er4->refs: usr
+ok 38 - er7->refs: usr
+ok 39 - svp->refs: global diag + er3 + er6
+ok 40 - svp->prev->refs: svp
+ok 41 - svp->refs: er3 + er6
+ok 42 - svp->prev->refs: svp
+ok 43 - svp->refs: er6
+ok 44 - svp->prev->refs: svp
+ *** diag_test: done ***
--
2.22.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v3 4/6] box: extend box.error.new({}) API
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
` (2 preceding siblings ...)
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 3/6] box: introduce stacked diagnostic area Kirill Shcherbatov
@ 2019-09-02 14:51 ` Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 5/6] iproto: refactor error encoding with mpstream Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 6/6] iproto: support stacked diagnostics for errors Kirill Shcherbatov
5 siblings, 0 replies; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-02 14:51 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, kostja, Kirill Shcherbatov
We need to extend box.error.new({}) to define complex errors
with diagnostics area in NetBox module in following patches.
Needed for #1148
@TarantoolBot document
Title: Extend box.error.new API
Create an error object, but do not throw. This is useful when error
information should be saved for later retrieval.
Currently the box.error.new() endpoint supports two usages:
box.error.new(errcode, format_str, <params>)
and
box.error.new({code = errcode, reason = error_msg_str,
<file = filename_str>, <line = linenumber>,
<prev = reason_error_object>})
The second one allows to (optionally) specify an extra information:
filename - the name of the file where the error was produced,
line - the line number,
prev - the previous (reason) error object.
Being set, the reason object could be unpacked using
box.error.prev API.
---
src/box/error.h | 5 +--
src/box/sql/vdbe.c | 2 +-
src/box/xrow.c | 2 +-
test/app-tap/module_api.c | 3 +-
test/box/function1.c | 29 ++++++++--------
test/box/reload1.c | 20 ++++++-----
test/box/reload2.c | 9 ++---
test/box/tuple_bench.c | 10 +++---
src/box/error.cc | 5 ++-
src/box/lua/error.cc | 15 +++++++--
test/box/misc.result | 70 +++++++++++++++++++++++++++++++++++++++
test/box/misc.test.lua | 22 ++++++++++++
12 files changed, 152 insertions(+), 40 deletions(-)
diff --git a/src/box/error.h b/src/box/error.h
index b8c7cf73d..2d5fb6370 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -119,6 +119,7 @@ box_error_clear(void);
* Set the last error.
*
* \param code IPROTO error code (enum \link box_error_code \endlink)
+ * \param prev - the reason error object
* \param format (const char * ) - printf()-like format string
* \param ... - format arguments
* \returns -1 for convention use
@@ -127,13 +128,13 @@ box_error_clear(void);
*/
int
box_error_set(const char *file, unsigned line, uint32_t code,
- const char *format, ...);
+ struct error *prev, const char *format, ...);
/**
* A backward-compatible API define.
*/
#define box_error_raise(code, format, ...) \
- box_error_set(__FILE__, __LINE__, code, format, ##__VA_ARGS__)
+ box_error_set(__FILE__, __LINE__, code, NULL, format, ##__VA_ARGS__)
/** \endcond public */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 02e16da19..627532265 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -939,7 +939,7 @@ case OP_Goto: { /* jump */
* text description of the error.
*/
case OP_SetDiag: { /* jump */
- box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);
+ box_error_set(__FILE__, __LINE__, pOp->p1, NULL, pOp->p4.z);
if (pOp->p2 != 0)
goto jump_to_p2;
break;
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 0ae5271c1..78462469a 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1063,7 +1063,7 @@ xrow_decode_error(struct xrow_header *row)
}
error:
- box_error_set(__FILE__, __LINE__, code, error);
+ box_error_set(__FILE__, __LINE__, code, NULL, error);
}
void
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index b81a98056..15078f953 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -202,7 +202,8 @@ int fiber_test_func(va_list va)
fiber_set_cancellable(true);
fiber_sleep(0.01);
if (fiber_is_cancelled()) {
- box_error_set(__FILE__, __LINE__, 10, "test error");
+ box_error_set(__FILE__, __LINE__, 10, NULL,
+ "test error");
return -1;
}
fiber_set_cancellable(false);
diff --git a/test/box/function1.c b/test/box/function1.c
index ee5a422b5..84ed1ea1d 100644
--- a/test/box/function1.c
+++ b/test/box/function1.c
@@ -17,13 +17,13 @@ args(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
uint32_t arg_count = mp_decode_array(&args);
if (arg_count < 1) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
- "invalid argument count");
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "invalid argument count");
}
if (mp_typeof(*args) != MP_UINT) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
- "first tuple field must be uint");
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "first tuple field must be uint");
}
uint32_t num = mp_decode_uint(&args);
@@ -70,7 +70,7 @@ divide(box_function_ctx_t *ctx, const char *args, const char *args_end)
return -1;
return box_return_tuple(ctx, tuple);
error:
- return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
"invalid argument");
}
@@ -90,9 +90,9 @@ multi_inc(box_function_ctx_t *ctx, const char *args, const char *args_end)
uint32_t index_id = box_index_id_by_name(space_id, INDEX_NAME,
strlen(INDEX_NAME));
if (space_id == BOX_ID_NIL || index_id == BOX_ID_NIL) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't find index %s in space %s",
- INDEX_NAME, SPACE_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't find index %s in space %s",
+ INDEX_NAME, SPACE_NAME);
}
say_debug("space_id = %u, index_id = %u", space_id, index_id);
@@ -104,7 +104,8 @@ multi_inc(box_function_ctx_t *ctx, const char *args, const char *args_end)
/* Decode next argument */
if (mp_typeof(*args) != MP_UINT)
return box_error_set(__FILE__, __LINE__,
- ER_PROC_C, "Expected uint keys");
+ ER_PROC_C, NULL,
+ "Expected uint keys");
uint32_t key = mp_decode_uint(&args);
(void) key;
@@ -125,8 +126,8 @@ multi_inc(box_function_ctx_t *ctx, const char *args, const char *args_end)
const char *field = box_tuple_field(tuple, 1);
if (field == NULL || mp_typeof(*field) != MP_UINT)
return box_error_set(__FILE__, __LINE__,
- ER_PROC_LUA,
- "Invalid tuple");
+ ER_PROC_LUA, NULL,
+ "Invalid tuple");
counter = mp_decode_uint(&field) + 1;
}
@@ -149,7 +150,7 @@ multi_inc(box_function_ctx_t *ctx, const char *args, const char *args_end)
int
errors(box_function_ctx_t *ctx, const char *args, const char *args_end)
{
- box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s", "Proc error");
+ box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL, "Proc error");
const box_error_t *error = box_error_last();
assert(strcmp(box_error_type(error), "ClientError") == 0);
@@ -184,8 +185,8 @@ test_yield(box_function_ctx_t *ctx, const char *args, const char *args_end)
uint32_t space_id = box_space_id_by_name(SPACE_NAME, strlen(SPACE_NAME));
if (space_id == BOX_ID_NIL) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't find space %s", SPACE_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't find space %s", SPACE_NAME);
}
assert(!box_txn());
diff --git a/test/box/reload1.c b/test/box/reload1.c
index 83227851e..232f5e297 100644
--- a/test/box/reload1.c
+++ b/test/box/reload1.c
@@ -13,9 +13,9 @@ foo(box_function_ctx_t *ctx, const char *args, const char *args_end)
uint32_t index_id = box_index_id_by_name(space_test_id, INDEX_NAME,
strlen(INDEX_NAME));
if (space_test_id == BOX_ID_NIL || index_id == BOX_ID_NIL) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't find index %s in space %s",
- INDEX_NAME, SPACE_TEST_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't find index %s in space %s",
+ INDEX_NAME, SPACE_TEST_NAME);
}
mp_decode_array(&args);
uint32_t num = mp_decode_uint(&args);
@@ -25,8 +25,9 @@ foo(box_function_ctx_t *ctx, const char *args, const char *args_end)
end = mp_encode_array(end, 1);
end = mp_encode_uint(end, num);
if (box_insert(space_test_id, buf, end, NULL) < 0) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't insert in space %s", SPACE_TEST_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't insert in space %s",
+ SPACE_TEST_NAME);
}
end = buf;
end = mp_encode_array(end, 1);
@@ -38,8 +39,9 @@ foo(box_function_ctx_t *ctx, const char *args, const char *args_end)
end = mp_encode_array(end, 1);
end = mp_encode_int(end, -((int)num));
if (box_insert(space_test_id, buf, end, NULL) < 0) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't insert in space %s", SPACE_TEST_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't insert in space %s",
+ SPACE_TEST_NAME);
}
return 0;
}
@@ -51,8 +53,8 @@ test_reload(box_function_ctx_t *ctx, const char *args, const char *args_end)
uint32_t space_id = box_space_id_by_name(SPACE_NAME, strlen(SPACE_NAME));
if (space_id == BOX_ID_NIL) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't find space %s", SPACE_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't find space %s", SPACE_NAME);
}
assert(!box_txn());
diff --git a/test/box/reload2.c b/test/box/reload2.c
index 30a359171..a3b99c8ae 100644
--- a/test/box/reload2.c
+++ b/test/box/reload2.c
@@ -10,16 +10,17 @@ foo(box_function_ctx_t *ctx, const char *args, const char *args_end)
uint32_t space_test_id = box_space_id_by_name(SPACE_TEST_NAME,
strlen(SPACE_TEST_NAME));
if (space_test_id == BOX_ID_NIL) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't find space %s", SPACE_TEST_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't find space %s", SPACE_TEST_NAME);
}
char buf[16];
char *end = buf;
end = mp_encode_array(end, 1);
end = mp_encode_uint(end, 0);
if (box_insert(space_test_id, buf, end, NULL) < 0) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't insert in space %s", SPACE_TEST_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't insert in space %s",
+ SPACE_TEST_NAME);
}
return 0;
}
diff --git a/test/box/tuple_bench.c b/test/box/tuple_bench.c
index ec92e6168..557b5d814 100644
--- a/test/box/tuple_bench.c
+++ b/test/box/tuple_bench.c
@@ -23,9 +23,9 @@ tuple_bench(box_function_ctx_t *ctx, const char *args, const char *args_end)
strlen(INDEX_NAME));
if (space_id == BOX_ID_NIL || index_id == BOX_ID_NIL) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C,
- "Can't find index %s in space %s",
- INDEX_NAME, SPACE_NAME);
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "Can't find index %s in space %s",
+ INDEX_NAME, SPACE_NAME);
}
say_debug("space_id = %u, index_id = %u", space_id, index_id);
@@ -37,8 +37,8 @@ tuple_bench(box_function_ctx_t *ctx, const char *args, const char *args_end)
/* get key types from args, and build test tuples with according types*/
uint32_t arg_count = mp_decode_array(&args);
if (arg_count < 1) {
- return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s",
- "invalid argument count");
+ return box_error_set(__FILE__, __LINE__, ER_PROC_C, NULL,
+ "invalid argument count");
}
uint32_t n = mp_decode_array(&args);
uint32_t knum = 0, kstr = 0;
diff --git a/src/box/error.cc b/src/box/error.cc
index 7dfe1b3ee..02ec9cafa 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -71,12 +71,15 @@ box_error_clear(void)
int
box_error_set(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...)
+ struct error *prev, const char *fmt, ...)
{
struct error *e = BuildClientError(file, line, ER_UNKNOWN);
ClientError *client_error = type_cast(ClientError, e);
if (client_error) {
client_error->m_errcode = code;
+ client_error->prev = prev;
+ if (prev != NULL)
+ error_ref(prev);
va_list ap;
va_start(ap, fmt);
error_vformat_msg(e, fmt, ap);
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 5a2f5fc7d..a3ce27aa8 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -49,6 +49,7 @@ luaT_error_create(lua_State *L, int top_base)
const char *reason = NULL;
const char *file = "";
unsigned line = 0;
+ struct error *prev = NULL;
lua_Debug info;
int top = lua_gettop(L);
if (top >= top_base && lua_type(L, top_base) == LUA_TNUMBER) {
@@ -82,6 +83,15 @@ luaT_error_create(lua_State *L, int top_base)
if (reason == NULL)
reason = "";
lua_pop(L, 1);
+ lua_getfield(L, top_base, "file");
+ file = lua_tostring(L, -1);
+ lua_pop(L, 1);
+ lua_getfield(L, top_base, "line");
+ line = lua_tonumber(L, -1);
+ lua_pop(L, 1);
+ lua_getfield(L, top_base, "prev");
+ prev = luaL_iserror(L, -1);
+ lua_pop(L, 1);
} else if (luaL_iserror(L, top_base)) {
lua_error(L);
return;
@@ -91,7 +101,8 @@ luaT_error_create(lua_State *L, int top_base)
}
raise:
- if (lua_getstack(L, 1, &info) && lua_getinfo(L, "Sl", &info)) {
+ if ((line == 0 || file == NULL) &&
+ lua_getstack(L, 1, &info) && lua_getinfo(L, "Sl", &info)) {
if (*info.short_src) {
file = info.short_src;
} else if (*info.source) {
@@ -102,7 +113,7 @@ raise:
line = info.currentline;
}
say_debug("box.error() at %s:%i", file, line);
- box_error_set(file, line, code, "%s", reason);
+ box_error_set(file, line, code, prev, "%s", reason);
}
static int
diff --git a/test/box/misc.result b/test/box/misc.result
index 555075a6c..8429bf191 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1328,3 +1328,73 @@ test_run:grep_log('default', 'test log')
---
- test log
...
+--
+-- gh-1148: Stacked diagnostics area in fiber
+--
+e1 = box.error.new({code = 128, file = "hohoho.c", line = 123, reason = "#1"})
+---
+...
+e2 = box.error.new({code = 129, file = "hohoho.c", line = 124, reason = "#2", prev = e1})
+---
+...
+e2.refs == 2
+---
+- true
+...
+e1.refs == 2
+---
+- true
+...
+box.error.prev(e1) == nil
+---
+- true
+...
+box.error.prev(e2) == e1
+---
+- true
+...
+e2.prev == box.error.prev(e2)
+---
+- true
+...
+collectgarbage()
+---
+- 0
+...
+e1.refs == 2
+---
+- true
+...
+e1.refs == 2
+---
+- true
+...
+box.error.clear()
+---
+...
+e1.refs == 2
+---
+- true
+...
+e2.refs == 1
+---
+- true
+...
+e2 = nil
+---
+...
+collectgarbage()
+---
+- 0
+...
+e1.refs == 1
+---
+- true
+...
+e1 = nil
+---
+...
+collectgarbage()
+---
+- 0
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index cc223b2ef..f4444b470 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -383,3 +383,25 @@ tuple_format = box.internal.new_tuple_format(format)
box.cfg{}
local ffi = require'ffi' ffi.C._say(ffi.C.S_WARN, nil, 0, nil, "%s", "test log")
test_run:grep_log('default', 'test log')
+
+--
+-- gh-1148: Stacked diagnostics area in fiber
+--
+e1 = box.error.new({code = 128, file = "hohoho.c", line = 123, reason = "#1"})
+e2 = box.error.new({code = 129, file = "hohoho.c", line = 124, reason = "#2", prev = e1})
+e2.refs == 2
+e1.refs == 2
+box.error.prev(e1) == nil
+box.error.prev(e2) == e1
+e2.prev == box.error.prev(e2)
+collectgarbage()
+e1.refs == 2
+e1.refs == 2
+box.error.clear()
+e1.refs == 2
+e2.refs == 1
+e2 = nil
+collectgarbage()
+e1.refs == 1
+e1 = nil
+collectgarbage()
--
2.22.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v3 5/6] iproto: refactor error encoding with mpstream
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
` (3 preceding siblings ...)
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 4/6] box: extend box.error.new({}) API Kirill Shcherbatov
@ 2019-09-02 14:51 ` Kirill Shcherbatov
2019-09-04 10:53 ` [tarantool-patches] " Konstantin Osipov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 6/6] iproto: support stacked diagnostics for errors Kirill Shcherbatov
5 siblings, 1 reply; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-02 14:51 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, kostja, Kirill Shcherbatov
Refactor iproto_reply_error and iproto_write_error with a new
mpstream-based helper mpstream_iproto_encode_error that encodes
error object for iproto protocol on a given stream object.
Previously each routine implemented an own error encoding, but
with the increasing complexity of encode operation with following
patches we need a uniform way to do it.
The iproto_write_error routine starts using region location
to use region-based mpstream. It is not a problem itself, because
errors reporting is not really performance-critical path.
Needed for #1148
---
src/box/xrow.c | 72 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 51 insertions(+), 21 deletions(-)
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 78462469a..e1e0a62be 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -36,12 +36,14 @@
#include "third_party/base64.h"
#include "fiber.h"
+#include "reflection.h"
#include "version.h"
#include "tt_static.h"
#include "error.h"
#include "vclock.h"
#include "scramble.h"
#include "iproto_constants.h"
+#include "mpstream.h"
static_assert(IPROTO_DATA < 0x7f && IPROTO_METADATA < 0x7f &&
IPROTO_SQL_INFO < 0x7f, "encoded IPROTO_BODY keys must fit into "\
@@ -381,10 +383,6 @@ static const struct iproto_body_bin iproto_body_bin = {
0x81, IPROTO_DATA, 0xdd, 0
};
-static const struct iproto_body_bin iproto_error_bin = {
- 0x81, IPROTO_ERROR, 0xdb, 0
-};
-
/** Return a 4-byte numeric error code, with status flags. */
static inline uint32_t
iproto_encode_error(uint32_t error)
@@ -475,46 +473,78 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot,
return 0;
}
+static void
+mpstream_error_handler(void *error_ctx)
+{
+ *(bool *)error_ctx = true;
+}
+
+static void
+mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
+{
+ mpstream_encode_map(stream, 2);
+ mpstream_encode_uint(stream, IPROTO_ERROR);
+ mpstream_encode_str(stream, error->errmsg);
+}
+
int
iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
uint32_t schema_version)
{
- uint32_t msg_len = strlen(e->errmsg);
- uint32_t errcode = box_error_code(e);
-
- struct iproto_body_bin body = iproto_error_bin;
char *header = (char *)obuf_alloc(out, IPROTO_HEADER_LEN);
if (header == NULL)
return -1;
+ /* The obuf-based stream has reserved area for header. */
+ bool is_error = false;
+ struct mpstream stream;
+ mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
+ mpstream_error_handler, &is_error);
+
+ uint32_t used = obuf_size(out);
+ mpstream_iproto_encode_error(&stream, e);
+ mpstream_flush(&stream);
+
+ uint32_t errcode = box_error_code(e);
iproto_header_encode(header, iproto_encode_error(errcode), sync,
- schema_version, sizeof(body) + msg_len);
- body.v_data_len = mp_bswap_u32(msg_len);
+ schema_version, obuf_size(out) - used);
+
/* Malformed packet appears to be a lesser evil than abort. */
- return obuf_dup(out, &body, sizeof(body)) != sizeof(body) ||
- obuf_dup(out, e->errmsg, msg_len) != msg_len ? -1 : 0;
+ return is_error ? -1 : 0;
}
void
iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
uint64_t sync)
{
- uint32_t msg_len = strlen(e->errmsg);
+ bool is_error = false;
+ struct mpstream stream;
+ struct region *region = &fiber()->gc;
+ mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
+ mpstream_error_handler, &is_error);
+
+ size_t region_svp = region_used(region);
+ mpstream_iproto_encode_error(&stream, e);
+ mpstream_flush(&stream);
+ if (is_error)
+ goto cleanup;
+
+ size_t payload_size = region_used(region) - region_svp;
+ char *payload = region_join(region, payload_size);
+ if (payload == NULL)
+ goto cleanup;
+
uint32_t errcode = box_error_code(e);
-
char header[IPROTO_HEADER_LEN];
- struct iproto_body_bin body = iproto_error_bin;
-
iproto_header_encode(header, iproto_encode_error(errcode), sync,
- schema_version, sizeof(body) + msg_len);
-
- body.v_data_len = mp_bswap_u32(msg_len);
+ schema_version, payload_size);
ssize_t unused;
unused = write(fd, header, sizeof(header));
- unused = write(fd, &body, sizeof(body));
- unused = write(fd, e->errmsg, msg_len);
+ unused = write(fd, payload, payload_size);
(void) unused;
+cleanup:
+ region_truncate(region, region_svp);
}
int
--
2.22.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v3 6/6] iproto: support stacked diagnostics for errors
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
` (4 preceding siblings ...)
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 5/6] iproto: refactor error encoding with mpstream Kirill Shcherbatov
@ 2019-09-02 14:51 ` Kirill Shcherbatov
5 siblings, 0 replies; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-02 14:51 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, kostja, Kirill Shcherbatov
Introduced a new iproto message key IPROTO_ERROR_V2 to support
errors stacked diagnostics. This key extends iproto protocol
messages to have the following structure:
{
// backward compatibility
IPROTO_ERROR: "the most recent error message",
// modern error message
IPROTO_ERROR_V2: {
{
// the most recent error object
"code": error_code_number,
"reason": error_reason_string,
"file": file_name_string,
"line": line_number,
},
...
{
// the oldest (reason) error object
},
}
}
Such messages are decoded with updated netbox client and applier.
The legacy format with IPROTO_ERROR is kept to provide backward
compatibility in distributed environment.
Closes #1148
---
src/box/iproto_constants.h | 1 +
src/box/xrow.c | 126 ++++++++++++++++++++++++++++++++-----
src/box/lua/net_box.lua | 24 ++++++-
test/box-py/iproto.result | 6 +-
test/box-py/iproto.test.py | 6 +-
test/box/misc.result | 82 ++++++++++++++++++++++++
test/box/misc.test.lua | 27 ++++++++
7 files changed, 248 insertions(+), 24 deletions(-)
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 724cce535..c190895c5 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -120,6 +120,7 @@ enum iproto_key {
* }
*/
IPROTO_SQL_INFO = 0x42,
+ IPROTO_ERROR_V2 = 0x43,
IPROTO_KEY_MAX
};
diff --git a/src/box/xrow.c b/src/box/xrow.c
index e1e0a62be..42b9f4f99 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -41,6 +41,7 @@
#include "tt_static.h"
#include "error.h"
#include "vclock.h"
+#include "mpstream.h"
#include "scramble.h"
#include "iproto_constants.h"
#include "mpstream.h"
@@ -479,12 +480,34 @@ mpstream_error_handler(void *error_ctx)
*(bool *)error_ctx = true;
}
+#define IPROTO_ERROR_V2_CODE "code"
+#define IPROTO_ERROR_V2_REASON "reason"
+#define IPROTO_ERROR_V2_FILE "file"
+#define IPROTO_ERROR_V2_LINE "line"
+
static void
mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
{
mpstream_encode_map(stream, 2);
mpstream_encode_uint(stream, IPROTO_ERROR);
mpstream_encode_str(stream, error->errmsg);
+
+ mpstream_encode_uint(stream, IPROTO_ERROR_V2);
+ uint32_t stack_sz = 0;
+ for (const struct error *it = error; it != NULL; it = it->prev)
+ stack_sz++;
+ mpstream_encode_array(stream, stack_sz);
+ for (const struct error *it = error; it != NULL; it = it->prev) {
+ mpstream_encode_map(stream, 4);
+ mpstream_encode_str(stream, IPROTO_ERROR_V2_CODE);
+ mpstream_encode_uint(stream, box_error_code(it));
+ mpstream_encode_str(stream, IPROTO_ERROR_V2_REASON);
+ mpstream_encode_str(stream, it->errmsg);
+ mpstream_encode_str(stream, IPROTO_ERROR_V2_FILE);
+ mpstream_encode_str(stream, it->file);
+ mpstream_encode_str(stream, IPROTO_ERROR_V2_LINE);
+ mpstream_encode_uint(stream, it->line);
+ }
}
int
@@ -1056,44 +1079,117 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len,
return 0;
}
+static int
+iproto_decode_error(const char **pos, uint32_t errcode, enum iproto_key *key)
+{
+ char reason[DIAG_ERRMSG_MAX] = {0};
+ char filename[DIAG_FILENAME_MAX] = {0};
+
+ *key = (enum iproto_key)mp_decode_uint(pos);
+ if (*key == IPROTO_ERROR && mp_typeof(**pos) == MP_STR) {
+ /* Legacy IPROTO_ERROR error. */
+ uint32_t len;
+ const char *str = mp_decode_str(pos, &len);
+ snprintf(reason, sizeof(reason), "%.*s", len, str);
+ box_error_set(__FILE__, __LINE__, errcode, NULL, reason);
+ return 0;
+ } else if (*key != IPROTO_ERROR_V2 || mp_typeof(**pos) != MP_ARRAY) {
+ /* Corrupted frame. */
+ return -1;
+ }
+
+ assert(*key == IPROTO_ERROR_V2 && mp_typeof(**pos) == MP_ARRAY);
+ struct error *prev = NULL;
+ uint32_t stack_sz = mp_decode_array(pos);
+ for (uint32_t stack_idx = 0; stack_idx < stack_sz; stack_idx++) {
+ filename[0] = reason[0] = 0;
+ uint32_t code = errcode, line = 0;
+ if (mp_typeof(**pos) != MP_MAP)
+ return -1;
+ uint32_t map_sz = mp_decode_map(pos);
+ for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
+ uint32_t len;
+ const char *str = mp_decode_str(pos, &len);
+ if (len == strlen(IPROTO_ERROR_V2_CODE) &&
+ memcmp(str, IPROTO_ERROR_V2_CODE, len) == 0) {
+ if (mp_typeof(**pos) != MP_UINT)
+ return -1;
+ code = mp_decode_uint(pos);
+ } else if (len == strlen(IPROTO_ERROR_V2_REASON) &&
+ memcmp(str, IPROTO_ERROR_V2_REASON, len) == 0) {
+ if (mp_typeof(**pos) != MP_STR)
+ return -1;
+ uint32_t len;
+ const char *str = mp_decode_str(pos, &len);
+ snprintf(reason, sizeof(reason), "%.*s",
+ len, str);
+ } else if (len == strlen(IPROTO_ERROR_V2_FILE) &&
+ memcmp(str, IPROTO_ERROR_V2_FILE,
+ len) == 0) {
+ if (mp_typeof(**pos) != MP_STR)
+ return -1;
+ uint32_t len;
+ const char *str = mp_decode_str(pos, &len);
+ snprintf(filename, sizeof(filename), "%.*s",
+ len, str);
+ } else if (len == strlen(IPROTO_ERROR_V2_LINE) &&
+ memcmp(str, IPROTO_ERROR_V2_LINE,
+ len) == 0) {
+ if (mp_typeof(**pos) != MP_UINT)
+ return -1;
+ line = mp_decode_uint(pos);
+ } else {
+ return -1;
+ }
+ }
+ box_error_set(filename[0] == 0 ? __FILE__ : filename,
+ line == 0 ? __LINE__ : line, code, prev, reason);
+ prev = diag_last_error(diag_get());
+ }
+ return 0;
+}
+
void
xrow_decode_error(struct xrow_header *row)
{
uint32_t code = row->type & (IPROTO_TYPE_ERROR - 1);
-
- char error[DIAG_ERRMSG_MAX] = { 0 };
- const char *pos;
- uint32_t map_size;
-
if (row->bodycnt == 0)
goto error;
- pos = (char *) row->body[0].iov_base;
+ const char *pos = (char *) row->body[0].iov_base;
if (mp_check(&pos, pos + row->body[0].iov_len))
goto error;
pos = (char *) row->body[0].iov_base;
if (mp_typeof(*pos) != MP_MAP)
goto error;
- map_size = mp_decode_map(&pos);
+ bool is_error_set = false;
+ uint32_t map_size = mp_decode_map(&pos);
for (uint32_t i = 0; i < map_size; i++) {
if (mp_typeof(*pos) != MP_UINT) {
mp_next(&pos); /* key */
mp_next(&pos); /* value */
continue;
}
- uint8_t key = mp_decode_uint(&pos);
- if (key != IPROTO_ERROR || mp_typeof(*pos) != MP_STR) {
+ enum iproto_key key;
+ if (iproto_decode_error(&pos, code, &key) != 0) {
mp_next(&pos); /* value */
continue;
}
-
- uint32_t len;
- const char *str = mp_decode_str(&pos, &len);
- snprintf(error, sizeof(error), "%.*s", len, str);
+ /*
+ * New tnt instances send both legacy
+ * IPROTO_ERROR and IPROTO_ERROR_V2 error
+ * messages. Prefer an error message is
+ * constructed with IPROTO_ERROR_V2 payload.
+ */
+ is_error_set = true;
+ if (key == IPROTO_ERROR_V2)
+ return;
+ assert(key == IPROTO_ERROR);
}
-
+ if (is_error_set)
+ return;
error:
- box_error_set(__FILE__, __LINE__, code, NULL, error);
+ box_error_set(__FILE__, __LINE__, code, NULL, NULL);
}
void
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 31a8c16b7..3d70d59b5 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -47,6 +47,7 @@ local IPROTO_ERROR_KEY = 0x31
local IPROTO_GREETING_SIZE = 128
local IPROTO_CHUNK_KEY = 128
local IPROTO_OK_KEY = 0
+local IPROTO_ERROR_V2_KEY = 0x43
-- select errors from box.error
local E_UNKNOWN = box.error.UNKNOWN
@@ -273,8 +274,20 @@ local function create_transport(host, port, user, password, callback,
--
function request_index:result()
if self.errno then
- return nil, box.error.new({code = self.errno,
- reason = self.response})
+ if type(self.response) == 'table' then
+ local prev = nil
+ for i = #self.response,1,-1 do
+ local error = self.response[i]
+ prev = box.error.new({code = error.code,
+ reason = error.reason,
+ file = error.file, line = error.line,
+ prev = prev})
+ end
+ return nil, prev
+ else
+ return nil, box.error.new({code = self.errno,
+ reason = self.response})
+ end
elseif not self.id then
return self.response
elseif not worker_fiber then
@@ -555,7 +568,12 @@ local function create_transport(host, port, user, password, callback,
body, body_end_check = decode(body_rpos)
assert(body_end == body_end_check, "invalid xrow length")
request.errno = band(status, IPROTO_ERRNO_MASK)
- request.response = body[IPROTO_ERROR_KEY]
+ if body[IPROTO_ERROR_V2_KEY] ~= nil then
+ request.response = body[IPROTO_ERROR_V2_KEY]
+ assert(type(request.response) == 'table')
+ else
+ request.response = body[IPROTO_ERROR_KEY]
+ end
request.cond:broadcast()
return
end
diff --git a/test/box-py/iproto.result b/test/box-py/iproto.result
index 900e6e24f..04e2e220c 100644
--- a/test/box-py/iproto.result
+++ b/test/box-py/iproto.result
@@ -169,9 +169,9 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
# Test bugs gh-272, gh-1654 if the packet was incorrect, respond with
# an error code and do not close connection
-sync=0, {49: 'Invalid MsgPack - packet header'}
-sync=1234, {49: "Missing mandatory field 'space id' in request"}
-sync=5678, {49: "Read access to space '_user' is denied for user 'guest'"}
+sync=0, Invalid MsgPack - packet header
+sync=1234, Missing mandatory field 'space id' in request
+sync=5678, Read access to space '_user' is denied for user 'guest'
space = box.schema.space.create('test_index_base', { id = 568 })
---
...
diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py
index 77637d8ed..cdd1a71c5 100644
--- a/test/box-py/iproto.test.py
+++ b/test/box-py/iproto.test.py
@@ -355,15 +355,15 @@ s = c._socket
header = { "hello": "world"}
body = { "bug": 272 }
resp = test_request(header, body)
-print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'])
+print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR))
header = { IPROTO_CODE : REQUEST_TYPE_SELECT }
header[IPROTO_SYNC] = 1234
resp = test_request(header, body)
-print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'])
+print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR))
header[IPROTO_SYNC] = 5678
body = { IPROTO_SPACE_ID: 304, IPROTO_KEY: [], IPROTO_LIMIT: 1 }
resp = test_request(header, body)
-print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'])
+print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR))
c.close()
diff --git a/test/box/misc.result b/test/box/misc.result
index 8429bf191..5fcbed1e5 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1398,3 +1398,85 @@ collectgarbage()
---
- 0
...
+-- test stacked diagnostics with netbox
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+---
+- true
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+lua_code = [[function(tuple)
+ local json = require('json')
+ return json.encode(tuple)
+ end]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+---
+...
+s = box.schema.space.create('withdata')
+---
+...
+pk = s:create_index('pk')
+---
+...
+idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
+---
+...
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+---
+...
+net = require('net.box')
+---
+...
+c = net.connect(os.getenv("LISTEN"))
+---
+...
+c.space.withdata:insert({1})
+---
+- error: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ can''t evaluate function'
+...
+e = box.error.last()
+---
+...
+e:unpack()
+---
+- code: 198
+ trace:
+ - file: <filename>
+ line: 68
+ type: ClientError
+ prev: '[string "return function(tuple) local ..."]:1: attempt to
+ call global ''require'' (a nil value)'
+ message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ can''t evaluate function'
+ refs: 3
+...
+e.prev:unpack()
+---
+- code: 32
+ trace:
+ - file: <filename>
+ line: 1010
+ type: ClientError
+ message: '[string "return function(tuple) local ..."]:1: attempt
+ to call global ''require'' (a nil value)'
+ refs: 4
+...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+---
+...
+s:drop()
+---
+...
+box.func.runtimeerror:drop()
+---
+...
+test_run:cmd("clear filter")
+---
+- true
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index f4444b470..195ed2882 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -405,3 +405,30 @@ collectgarbage()
e1.refs == 1
e1 = nil
collectgarbage()
+
+-- test stacked diagnostics with netbox
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+test_run:cmd("setopt delimiter ';'")
+lua_code = [[function(tuple)
+ local json = require('json')
+ return json.encode(tuple)
+ end]]
+test_run:cmd("setopt delimiter ''");
+box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+s = box.schema.space.create('withdata')
+pk = s:create_index('pk')
+idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
+
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+net = require('net.box')
+c = net.connect(os.getenv("LISTEN"))
+c.space.withdata:insert({1})
+
+e = box.error.last()
+e:unpack()
+e.prev:unpack()
+
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+s:drop()
+box.func.runtimeerror:drop()
+test_run:cmd("clear filter")
--
2.22.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v3 5/6] iproto: refactor error encoding with mpstream
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 5/6] iproto: refactor error encoding with mpstream Kirill Shcherbatov
@ 2019-09-04 10:53 ` Konstantin Osipov
0 siblings, 0 replies; 11+ messages in thread
From: Konstantin Osipov @ 2019-09-04 10:53 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches, georgy, alexander.turenko
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/09/02 17:55]:
> +static void
> +mpstream_error_handler(void *error_ctx)
> +{
> + *(bool *)error_ctx = true;
> +}
> +
> +static void
> +mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
I think the name should be encode_error_in_mpstream().
Basically, the convention is:
- if it is an mpstream method, its name is mpstream_verb_subject
- if it is error method, the name is error_verb_subject
- if it is a standalone function, the name is
verb_subject_.._subject.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
@ 2019-09-09 8:13 ` Kirill Shcherbatov
2019-09-09 19:44 ` Vladislav Shpilevoy
2019-09-09 19:44 ` Vladislav Shpilevoy
1 sibling, 1 reply; 11+ messages in thread
From: Kirill Shcherbatov @ 2019-09-09 8:13 UTC (permalink / raw)
To: tarantool-patches, georgy; +Cc: alexander.turenko, Vladislav Shpilevoy
Hi!
1.
> Sorry, but I couldn't match these two calls:
> box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
> and
> box.error.new({code = user_code, reason = user_error_msg})
Currently the box.error.new() endpoint supports two usages:
box.error.new(errcode, format_str, <params>)
and
box.error.new({code = errcode, reason = error_msg_str})
Tarantool distinguishes these two usages by argument type.
In scope of the patch I extend the second interface with new parameters to construct
more informative error object on the client side (in net.box module)
box.error.new({code = errcode, reason = error_msg_str,
<file = filename_str>, <line = linenumber>,
<prev = reason_error_object>})
2.
>> +```
>> +
>> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`,
>> `:__serialize()` methods and uniform `.type`, `.message` and `.trace`
>> fields. +
>> +
> What if we want to throw a table error - as vshard does.
https://github.com/tarantool/vshard/blob/master/vshard/error.lua
As fores I've understood, vshard has an own error representation same as box_error
adapter that constructs vshard-compatible table error object.
The backward compatibility wouldn't be broken with introducing a couple new fields in core error object.
However, to have a profit with new stacked diagnostics functionality the box_error adapter should be
updated correspondingly. This likely requires extending vshard error functionality.
I'd better create a related ticket for vshard module, if you are agree with me.
> Additionally,if we would like to talk about user define errors we should
> consider an API to register user error classes. Also it could be worth to
> uncover error code/name clashing causes (both raising and catching).
At first, such problem should be discussed taking into account the following ticket:
https://github.com/tarantool/tarantool/issues/4398
Perhaps, we should introduce a new system space _errors to define new error classes.
This must work in replicating environment. Extending of iproto IPROTO_ERROR_V2 wouldn't
be a problem: we just need to introduce a new key "type" to transfer error class.
Guess, it needn't be implemented as a part of #1148, right?
3.
> I don't see any valid purpose for svp* methods. Please explain the cases when
> we need to rollback a diag area.
Kostya had a strong opinion about an ability to reset a last-errors set.
I am able to imagine a situation where this could be useful.
Some APIs may set errors inside while this error is not usefull, because
we've called such function to get the result only:
1) space_cache_find returns space with a given id, actualize the space cache or
returns NULL and sets the error.
Most of them user_find/user_by_id, user_find_by_name etc. has two interfaces:
that sets the error and that doesn't. Perhaps, sometimes reseting the error set
(understanding why it is necessary) is better than forgetting to set the error at
all.
Actually, I don't know, but I really like svp-like API.
>> Let's introduce
>> `diag_svp(diag) -> SVP`, +`diag_rollback_to_svp(diag, SVP)`. The
>> implementation proposal for SVP structure is given below: +
>> +```
>> + truncate
>> + xxxxxxxxxxxxxxxxxxxxxxxx SVP
>> +DIAG: +-------+ |
>> + | err | |
>> + +-------+----->+-------+ here
>> + third prev | err |
>> + +-------+------->+-------+
>> + second prev | err |
>> + +-------+
>> + first
>> +```
>> +
>> +The diag_structure already has `struct error *next` pointer;
> It's called `last' if I didn't mistaken .
>> +
>> +Let's introduce a similar pointer in an error structure to organize a
>> reason list: +```
>> +struct error {
>> + ...
>> + struct error *prev;
> I would prefer if you called it reason.
I completely agree with you, I also like 'reason' name more, but
box.error.new({code = ..., reason = ..}) already use this name in other
meaning (error message). So I've decided to use prev everywhere to avoid
a mess.
4.
> What with err->log() method? Would we change error formatting and logging if
> an error has a reason?
We've decided do not change log method earlier.
Moreover, we decided do not change :unpack() method significantly (because of third-party
modules like vshard that has assumptions about their output format).
5.
>> +-- Return a reason error object for given error
>> +-- (when exists, nil otherwise)
>> +box.error.prev(error) == error.prev
>> +```
> You let out of scope standard lua "error()" call. Please take it into
> consideration.
I can't say nothing about error() call. It is not box error, it can't be extended.
It should be deprecated when #4398 would be closed, right?
6.
>> +Let's extend existent binary protocol with a new key IPROTO_ERROR_V2:
>> +{
>> + // backward compatibility
>> + IPROTO_ERROR: "the most recent error message",
>> + // modern error message
>> + IPROTO_ERROR_V2: {
>> + {
>> + // the most recent error object
>> + "code": error_code_number,
>> + "reason": error_reason_string,
>> + "file": file_name_string,
>> + "line": line_number,
>> + },
>> + ...
>> + {
>> + // the oldest (reason) error object
>> + },
>> + }
>> +}
Is this format acceptable for you?
Kostya talked a bit about "deprecation plan". Maybe it worth to be discussed.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool
2019-09-09 8:13 ` [tarantool-patches] " Kirill Shcherbatov
@ 2019-09-09 19:44 ` Vladislav Shpilevoy
0 siblings, 0 replies; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-09-09 19:44 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches, georgy; +Cc: alexander.turenko
> 2.
>>> +```
>>> +
>>> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`,
>>> `:__serialize()` methods and uniform `.type`, `.message` and `.trace`
>>> fields. +
>>> +
>> What if we want to throw a table error - as vshard does.
>
> https://github.com/tarantool/vshard/blob/master/vshard/error.lua
> As fores I've understood, vshard has an own error representation same as box_error
> adapter that constructs vshard-compatible table error object.
VShard repeats error:unpack() format. If you change result of unpack(), then
I will update VShard (if that will be necessary).
> The backward compatibility wouldn't be broken with introducing a couple new fields in core error object.
Yes, if you only add new fields to error:unpack(), then nothing
will break.
> However, to have a profit with new stacked diagnostics functionality the box_error adapter should be
> updated correspondingly. This likely requires extending vshard error functionality.
> I'd better create a related ticket for vshard module, if you are agree with me.
>
>
> 3.
>> I don't see any valid purpose for svp* methods. Please explain the cases when
>> we need to rollback a diag area.
Me neither. Svp won't solve any problems IMO. It will
just complicate the code. Talking of cases below - if you
call a function, you need to know how it works. If you called
a function not setting a diag, and you expected a diag, that
you either didn't test your code properly, or you was
inattentive, or both. Svp won't solve neither of that problems.
You still may forget to reset svp, to set svp, to test the code.
> Kostya had a strong opinion about an ability to reset a last-errors set.
Honestly, I didn't understand his point. And I think, if
we ever will understand, we can easily add svps. But if we add
them now and they appear to be useless, then we won't be able
to drop them, because of compatibility.
> 5.
>>> +-- Return a reason error object for given error
>>> +-- (when exists, nil otherwise)
>>> +box.error.prev(error) == error.prev
>>> +```
>> You let out of scope standard lua "error()" call. Please take it into
>> consideration.
>
> I can't say nothing about error() call. It is not box error, it can't be extended.
> It should be deprecated when #4398 would be closed, right?
error() is a built-in Lua function, it is a part of the language. You can't
deprecate it. But I also don't understand what is wrong with it. Seems like
it will work just like always.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
2019-09-09 8:13 ` [tarantool-patches] " Kirill Shcherbatov
@ 2019-09-09 19:44 ` Vladislav Shpilevoy
1 sibling, 0 replies; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-09-09 19:44 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov, georgy; +Cc: alexander.turenko, kostja
Hi!
First of all, great RFC, this is getting better and better!
> +### Binary protocol
> +Currently errors are sent as `(IPROTO_ERROR | errcode)` messages with an error's message string payload.
> +
> +We must to design a backward-compatible error transfer specification. Imagine a net.box old-new Tarantool connection, same as existing connector: they mustn't broken.
> +
> +Let's extend existent binary protocol with a new key IPROTO_ERROR_V2:
I would rather rename the old one to IPROTO_ERROR_16, just like we
did with IPROTO_CALL_16. And the new one will be IPROTO_ERROR.
> +{
> + // backward compatibility
> + IPROTO_ERROR: "the most recent error message",
> + // modern error message
> + IPROTO_ERROR_V2: {
> + {
> + // the most recent error object
> + "code": error_code_number,
Please, don't use strings as map keys, you will pad out the
protocol. It is a binary proto, not text. Just use numeric
constants: IPROTO_ERROR_CODE = 0x01, IPROTO_ERROR_REASON = 0x02,
etc.
> + "reason": error_reason_string,
> + "file": file_name_string,
> + "line": line_number,
> + },
> + ...
> + {
> + // the oldest (reason) error object
> + },
> + }
> +}
> +
> +### SQL Warnings
> +SQL Standard defines ["WARNINGS"](https://docs.microsoft.com/en-us/sql/t-sql/statements/set-ansi-warnings-transact-sql?view=sql-server-2017) term:
> +- When set to ON, if null values appear in aggregate functions, such as SUM, AVG, MAX, MIN, STDEV, STDEVP, VAR, VARP, or COUNT, a warning message is generated. When set to OFF, no warning is issued.
> +- When set to ON, the divide-by-zero and arithmetic overflow errors cause the statement to be rolled back and an error message is generated. When set to OFF, the divide-by-zero and arithmetic overflow errors cause null values to be returned. The behavior in which a divide-by-zero or arithmetic overflow error causes null values to be returned occurs if an INSERT or UPDATE is tried on a character, Unicode, or binary column in which the length of a new value exceeds the maximum size of the column. If SET ANSI_WARNINGS is ON, the INSERT or UPDATE is canceled as specified by the ISO standard. Trailing blanks are ignored for character columns and trailing nulls are ignored for binary columns. When OFF, data is truncated to the size of the column and the statement succeeds.
> +
> +According to the current convention, all Tarantool's function use "return not null error code on error" convention. Iff the global diagnostic area is valid (actually, Unix errno uses the same semantics). So we need an additional `Parser`/`VDBE` marker to indicate that the warning diagnostics
> +is set. Say, in addition to `bool is_aborted` field we may introduce and additional field `bool is_warning_set`.
> +
> +To avoid a mess between SQL warnings and real errors let's better introduce an
> +"diagnostics"-semantics area in Parser/VDBE classes where all warnings are
> +organized in a list. Internal warning representation needn't follow error
> +specification and may use other representation when it is reasonable.
>
I agree, warnings have nothing to do with non-SQL methods, and should
be kept for SQL queries only if possible.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-09-09 19:40 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-02 14:51 [tarantool-patches] [PATCH v3 0/6] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 1/6] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
2019-09-09 8:13 ` [tarantool-patches] " Kirill Shcherbatov
2019-09-09 19:44 ` Vladislav Shpilevoy
2019-09-09 19:44 ` Vladislav Shpilevoy
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 2/6] box: rename diag_add_error to diag_set_error Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 3/6] box: introduce stacked diagnostic area Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 4/6] box: extend box.error.new({}) API Kirill Shcherbatov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 5/6] iproto: refactor error encoding with mpstream Kirill Shcherbatov
2019-09-04 10:53 ` [tarantool-patches] " Konstantin Osipov
2019-09-02 14:51 ` [tarantool-patches] [PATCH v3 6/6] iproto: support stacked diagnostics for errors Kirill Shcherbatov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox