From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4141C24DBB for ; Mon, 5 Aug 2019 17:16:28 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id giifHk-9eqwI for ; Mon, 5 Aug 2019 17:16:28 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D0CAB24DA3 for ; Mon, 5 Aug 2019 17:16:27 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API References: <47d9072e60bdc563c7466a4e51db1a61bc71a610.1564657285.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <55d56797-f937-4088-fdba-7d9dfef51ae6@tarantool.org> Date: Mon, 5 Aug 2019 23:18:48 +0200 MIME-Version: 1.0 In-Reply-To: <47d9072e60bdc563c7466a4e51db1a61bc71a610.1564657285.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov Cc: alexander.turenko@tarantool.org, kostja@tarantool.org Thanks for the patch! See 11 comments below. On 01/08/2019 13:13, Kirill Shcherbatov wrote: > Closes #1148 > > @TarantoolBot document > Title: Stacked diagnostics area in fiber > > Tarantool errors subsystem had been refactored to support stacked 1. 'error subsystem'. 2. 'has been'. > errors. Errors in Tarantool are represented as a > cdata objects with following fields and methods > > error.type - get top-level error type > error.message - get top-level error description string > error.trace - get top-level file and line trace for error object > error.code - get top-level error code > > error:raise() - raise error exception > error:match() - match a string in error message with given > pattern. Required to uniformly handle string > error messages and cdata error objects: > f1() return nil, "err1" end > f2() return nil, box.error.new(555, "err2") end > _, err1 = f1() > -, err2 = f2() > err1:match("err1") == true > err2:match("err2") == true > > -- Methods with stacked error semantics > error:unpack() - return error's detailed stack diagnostics: > a table of errors in their historical order > with all related information > error:has(code)- test whether a given error code is met in > error's stacked representation > err1 = > err1:wrap(err) - add err error object as a reason for err1 object > this call modifies err1 object and doesn't > modify err object. 3. If it modifies err1, then why do I need to assign a result to err1? The same below. > err1_parent = > err1:unwrap() - remove the most recent error in error object, > return it's parent. the call has no effect when > there is no parent in given error object. > > Example: > errors = {ERR_IO = 1001, ERR_USER_CREATE = 1002} > e0 = box.error.new(errors.ERR_IO, 'IO error') > e1 = box.error.new(errors.ERR_USER_CREATE, 'Can\'t create a user') > e1 = e1:wrap(e0) > e1:unpack() > - - type: ClientError > message: Unknown error > code: 1001 > trace: > - file: '[string "e0 = box.error.new(errors.ERR_IO, ''IO error'')"]' > line: 1 > - type: ClientError > message: Unknown error > code: 1002 > trace: > - file: '[string "e1 = box.error.new(errors.ERR_USER_CREATE, > ''C..."]' > line: 1 > e1:has(errors.ERR_IO) > - true > e1:has(errors.ERR_USER_CREATE) > - true > --- > diff --git a/src/box/error.h b/src/box/error.h > index b8c7cf73d..f45a0a661 100644 > --- a/src/box/error.h > +++ b/src/box/error.h > @@ -85,6 +85,29 @@ box_error_code(const box_error_t *error); > const char * > box_error_message(const box_error_t *error); > > +/** > + * Wrap reason error object into given error. > + * This API replaces box.error.last() value with an updated > + * error object. 4. Why do you need to change the global error? And why is not it mentioned in the docbot request? > + * \param error wrapper error object > + * \param reason error object > + * \return a pointer to the updated error object > + */ > +struct error * > +box_error_wrap(box_error_t *error, box_error_t *reason); > + > +/** > + * Removes the element's parent and returns the > + * unwrapped reason error object. > + * This API replases box.error.last() value with an unwrapped 5. 'replaces'. > + * reason reason error object. The original error object is > + * modified an has no reason anymore. > + * \param error error object > + * \return a pointer to the updated error object > + */ > +struct error * > +box_error_unwrap(box_error_t *error); > + > /** > * Get the information about the last API call error. > * > diff --git a/src/box/error.cc b/src/box/error.cc > index 7dfe1b3ee..4ed2a995b 100644 > --- a/src/box/error.cc > +++ b/src/box/error.cc > @@ -57,6 +57,31 @@ box_error_message(const box_error_t *error) > return error->errmsg; > } > > +struct error * > +box_error_wrap(box_error_t *error, box_error_t *reason) > +{ > + if (reason == NULL) { > + diag_set_error(diag_get(), error); > + return error; > + } > + assert(reason != NULL && error->reason == NULL); > + error_ref(reason); > + diag_set_error(diag_get(), error); > + error->reason = reason; > + return error; > +} > + > +struct error * 6. Please, return box_error_t * here and above. > +box_error_unwrap(box_error_t *error) > +{ > + struct error *reason = error->reason; > + assert(reason != NULL); > + diag_set_error(diag_get(), reason); > + error_unref(reason); > + error->reason = NULL; > + return reason; 7. Unwrap does not allow to unwrap a leaf error. But there is no API to determine if the error is leaf. So a user can't determine when to stop calling unwrap. I am talking about C public API which you have changed here. A user can't check error->reason != NULL before calling box_error_unwrap. Moreover, it is inconsistent with Lua version. Lets better return the argument when error->reason == NULL in box_error_unwrap. Then a user of the C API would just unwrap the stack until box_error_unwrap(e) == e. Also it simplifies Lua version implementation. > +} > + > box_error_t * > box_error_last(void) > { > diff --git a/test/box/errors.result b/test/box/errors.result > new file mode 100644 > index 000000000..b0fb2f38d > --- /dev/null > +++ b/test/box/errors.result > @@ -0,0 +1,265 @@ > +-- test-run result file version 2 > +env = require('test_run') > + | --- > + | ... > +test_run = env.new() > + | --- > + | ... > + 8. On the branch here is a filter. Please, add a filter for line numbers too. > +-- > +-- gh-1148: Stacked diagnostics area in fiber > +-- > +s = box.schema.space.create('withdata') > + | --- > + | ... > +pk = s:create_index('pk') > + | --- > + | ... > +lua_code = [[function(tuple) return {{"hello", "world"}, {1, 2}} end]] > + | --- > + | ... > +box.schema.func.create('invalidreturn3', {body = lua_code, is_deterministic = true, is_sandboxed = true, opts = {is_multikey = true}}) > + | --- > + | ... > +idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'unsigned'}, {2, 'unsigned'}}}) > + | --- > + | ... > +tuple, err2 = pcall(box.internal.insert, s.id, {1}) > + | --- > + | ... > +assert(tuple == false) > + | --- > + | - true > + | ... > +err2:has(box.error.KEY_PART_TYPE) > + | --- > + | - true > + | ... > +err2:has(box.error.FUNC_INDEX_FORMAT) > + | --- > + | - true > + | ... > +err2:has(box.error.PROC_LUA) > + | --- > + | - false > + | ... > +err2.trace > + | --- > + | - - file: /home/kir/WORK/tarantool/src/box/key_list.c > + | line: 175 > + | ... > + > +-- Test wrap/unwrap cases and garbage collection > +err3 = box.error.new(box.error.PROC_LUA, "Can't initialize storage") > + | --- > + | ... > +err3:wrap(err2) > + | --- > + | - Can't initialize storage > + | ... > +err2:has(box.error.PROC_LUA) > + | --- > + | - false > + | ... > +err4 = box.error.new(box.error.PROC_LUA, "Can't initialize storage 2") > + | --- > + | ... > +err4:wrap(nil) > + | --- > + | - Can't initialize storage 2 > + | ... > +err4:unpack() > + | --- > + | - - type: ClientError > + | message: Can't initialize storage 2 > + | code: 32 > + | trace: > + | - file: '[string "err4 = box.error.new(box.error.PROC_LUA, "Can..."]' > + | line: 1 > + | ... > +err4:wrap(err3) > + | --- > + | - Can't initialize storage 2 > + | ... > +err4:wrap(nil) > + | --- > + | - error: 'builtin/error.lua:157: The error:wrap() method is only applicable for errors > + | with no reason defined' 9. This filename is not filtered on the branch. > + | ... > +collectgarbage() > + | --- > + | - 0 > + | ... > +box.error.clear() > + | --- > + | ... > +err2:unpack() > + | --- > + | - - type: ClientError > + | message: 'Supplied key type of part 0 does not match index part type: expected > + | unsigned' > + | code: 18 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_def.h > + | line: 534 > + | - type: ClientError > + | message: 'Key format doesn''t match one defined in functional index ''idx'' of > + | space ''withdata'': key does not follow functional index key definition' > + | code: 199 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_list.c > + | line: 175 > + | ... > +err3:unpack() > + | --- > + | - - type: ClientError > + | message: 'Supplied key type of part 0 does not match index part type: expected > + | unsigned' > + | code: 18 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_def.h > + | line: 534 > + | - type: ClientError > + | message: 'Key format doesn''t match one defined in functional index ''idx'' of > + | space ''withdata'': key does not follow functional index key definition' > + | code: 199 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_list.c > + | line: 175 > + | - type: ClientError > + | message: Can't initialize storage > + | code: 32 > + | trace: > + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]' > + | line: 1 > + | ... > +err4:unpack() > + | --- > + | - - type: ClientError > + | message: 'Supplied key type of part 0 does not match index part type: expected > + | unsigned' > + | code: 18 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_def.h > + | line: 534 > + | - type: ClientError > + | message: 'Key format doesn''t match one defined in functional index ''idx'' of > + | space ''withdata'': key does not follow functional index key definition' > + | code: 199 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_list.c > + | line: 175 > + | - type: ClientError > + | message: Can't initialize storage > + | code: 32 > + | trace: > + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]' > + | line: 1 > + | - type: ClientError > + | message: Can't initialize storage 2 > + | code: 32 > + | trace: > + | - file: '[string "err4 = box.error.new(box.error.PROC_LUA, "Can..."]' > + | line: 1 > + | ... > + > +err4 = nil > + | --- > + | ... > +collectgarbage() > + | --- > + | - 0 > + | ... > +err2:unpack() > + | --- > + | - - type: ClientError > + | message: 'Supplied key type of part 0 does not match index part type: expected > + | unsigned' > + | code: 18 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_def.h > + | line: 534 > + | - type: ClientError > + | message: 'Key format doesn''t match one defined in functional index ''idx'' of > + | space ''withdata'': key does not follow functional index key definition' > + | code: 199 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_list.c > + | line: 175 > + | ... > +err3:unpack() > + | --- > + | - - type: ClientError > + | message: 'Supplied key type of part 0 does not match index part type: expected > + | unsigned' > + | code: 18 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_def.h > + | line: 534 > + | - type: ClientError > + | message: 'Key format doesn''t match one defined in functional index ''idx'' of > + | space ''withdata'': key does not follow functional index key definition' > + | code: 199 > + | trace: > + | - file: /home/kir/WORK/tarantool/src/box/key_list.c > + | line: 175 > + | - type: ClientError > + | message: Can't initialize storage > + | code: 32 > + | trace: > + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]' > + | line: 1 > + | ... > + > +err3_head = err3 > + | --- > + | ... > +err3 = err3:unwrap() > + | --- > + | ... > +err3 == err2 > + | --- > + | - true > + | ... > +box.error.last() == err3 > + | --- > + | - true > + | ... > +err3_head:unpack() > + | --- > + | - - type: ClientError > + | message: Can't initialize storage > + | code: 32 > + | trace: > + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]' > + | line: 1 > + | ... > +err3_head = nil > + | --- > + | ... > +collectgarbage() > + | --- > + | - 0 > + | ... > + > +box.error.clear() > + | --- > + | ... > +err3 = nil > + | --- > + | ... > +collectgarbage() > + | --- > + | - 0 > + | ... > +err2 = nil > + | --- > + | ... > +collectgarbage() > + | --- > + | - 0 > + | ... 10. Nit: you could nullify all the errors at once, and call collectgarbage. > + > +s:drop() > + | --- > + | ... 11. In the RFC you said, that IProto returns a list of error. Where it is?