[tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Aug 6 00:18:48 MSK 2019
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<struct error *> 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?
More information about the Tarantool-patches
mailing list