Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 7 comments below.

On 25/03/2020 02:43, Nikita Pettik wrote:
> In terms of implementation, now struct error objects can be organized
> into double-linked lists. To achieve this pointers to the next and
> previous elements (cause and effort correspondingly) have been added to

1. effort -> effect.

> struct error. It is worth mentioning that already existing rlist and
> stailq list implementations are not suitable: rlist is cycled list, as a
> result it is impossible to start iteration over the list from random
> list entry and finish it at the logical end of the list; stailq is
> single-linked list leaving no possibility to remove elements from the
> middle of the list.
> 
> diff --git a/src/box/key_list.c b/src/box/key_list.c
> index 3d736b55f..81eb501a5 100644
> --- a/src/box/key_list.c
> +++ b/src/box/key_list.c
> @@ -64,7 +64,7 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  		/* 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) : "",
> +			 space != NULL ? space_name(space) : "",
>  			 diag_last_error(diag_get())->errmsg);
>  		return -1;
>  	}
> @@ -75,7 +75,7 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  		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) : "",
> +			 space != NULL ? space_name(space) : "",
>  			 diag_last_error(diag_get())->errmsg);
>  		return -1;
>  	}
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index f1bbde7f0..92575374d 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -688,8 +688,8 @@ func_persistent_lua_load(struct func_lua *func)
>  		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);
> +				 func->base.def->name,
> +				 diag_last_error(diag_get())->errmsg);
>  			goto end;

2. Do you really need the 3 hunks above? Seems like unnecessary
refactoring.

>  		}
>  	} else {
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index c350abb4a..57da5da44 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -34,6 +34,43 @@
>  /* Must be set by the library user */
>  struct error_factory *error_factory = NULL;
>  
> +int
> +error_set_prev(struct error *e, struct error *prev)
> +{
> +	/*
> +	 * Make sure that adding error won't result in cycles.
> +	 * Don't bother with sophisticated cycle-detection
> +	 * algorithms, simple iteration is OK since as a rule
> +	 * list contains a dozen errors at maximum.
> +	 */
> +	struct error *tmp = prev;
> +	while (tmp != NULL) {
> +		if (tmp == e)
> +			return -1;
> +		tmp = tmp->cause;
> +	}

3. Or we could remove 'prev' from its current stack gracefully,
since the list is doubly linked. Then cycles become impossible.
Is there a reason not to do that? I mean, we remove 'prev'
from its old stack by changing its 'effect' already anyway.

Can do the same for the other pointer, and no need to check
for cycles anymore.

I can only think of a problem when someone makes

    e1 = box.error.new()
    e2 = box.error.new()
    e2:set_prev(e1)

and then

    e4 = box.error.new()
    e4:set_prev(e1)

assuming that e1 will be copied. But it will steal e1 from
its old stack. Even with your current solution. Maybe to make
it more protected against dummies we should forbid moving error
from one stack to another? So an error can't change its 'cause'
and 'effect' once they become not NULL. In that way we eliminate
any uncertainties and in future can decide whether we want to copy
in set_prev() when necessary, or move from previous stack. Or we
will keep this forbidden because probably no one does moving
between stacks, nor the example I showed above. This also
significantly simplifies tests and reduces probability of having
a bug there. What are your thoughts?

> +	/*
> +	 * At once error can feature only one reason.
> +	 * So unlink previous 'cause' node.
> +	 */
> +	if (e->cause != NULL) {
> +		e->cause->effect = NULL;
> +		error_unref(e->cause);
> +	}
> +	/* Set new 'prev' node. */
> +	e->cause = prev;
> +	/*
> +	 * Unlink new 'effect' node from its old list of 'cause'
> +	 * errors. nil can be also passed as an argument.
> +	 */
> +	if (prev != NULL) {
> +		error_unlink_effect(prev);
> +		prev->effect = e;
> +		error_ref(prev);

4. Better call error_ref() before error_unlink_effect(). To
make it impossible for prev to be deleted inside unlink() and
be used after free. I can't find how can be it done now, but by
moving ref() a bit above we will make it impossible in future
as well.

> +	}
> +	return 0;
> +}
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 7e1e1a174..675b9c6f1 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h> @@ -96,11 +113,56 @@ static inline void
>  
> +/**
> + * Set previous error: cut @a prev from its previous 'tail' of
> + * causes and link to the one @a e belongs to. Note that all
> + * previous errors starting from @a prev->cause are transferred
> + * with it as well (i.e. causes for given error are not erased).
> + * For instance:
> + * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
> + * e2:set_effect(e3): e1 -> e2 -> e3 -> e4 -> NULL
> + *
> + * @a effect can be  NULL. To be used as ffi method in lua/error.lua.

5. Out of 66 and double whitespace before 'NULL'.

> + *
> + * @retval -1 in case adding @a effect results in list cycles;
> + *          0 otherwise.
> + */
> +int
> +error_set_prev(struct error *e, struct error *prev);
> +
> diff --git a/test/box/error.result b/test/box/error.result
> index a19a7044f..ff2b8b270 100644
> --- a/test/box/error.result
> +++ b/test/box/error.result
> @@ -502,6 +502,290 @@ box.error()
> +-- In case error is destroyed, it unrefs reference counter
> +-- of its previous error. In turn, box.error.clear() refs/unrefs
> +-- only head and doesn't touch other errors.
> +--
> +e2:set_prev(nil)
> + | ---
> + | ...
> +box.error.set(e1)
> + | ---
> + | ...
> +assert(box.error.last() == e1)
> + | ---
> + | - true
> + | ...
> +assert(box.error.last().prev == e2)
> + | ---
> + | - true
> + | ...
> +box.error.clear()
> + | ---
> + | ...
> +assert(box.error.last() == nil)
> + | ---
> + | - true
> + | ...
> +assert(e1.prev == e2)
> + | ---
> + | - true
> + | ...
> +assert(e2.code  == 111)

6. Double whitespace. Macbook keyboard, huh?

> diff --git a/test/engine/func_index.result b/test/engine/func_index.result
> index 84cb83022..159158f1c 100644
> --- a/test/engine/func_index.result
> +++ b/test/engine/func_index.result
> @@ -261,12 +261,6 @@ box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true
>  idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
>   | ---
>   | ...
> -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)'
> - | ...
>  idx:drop()

7. Why do you drop it here? The commit does not change any
existing places setting errors. So it should not lead to
any test changes.

>   | ---
>   | ...