[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