[Tarantool-patches] [PATCH] box: always promote error created via box.error() to diag

[Tarantool-patches] [PATCH] box: always promote error created via box.error() to diag

[Nikita Pettik]

Note that it is vital that box.error() now promotes error to diag, since
otherwise user is unable to set to diag custom error, which in turn is
linked with other errors:

e1 = box.error.new({code = 111, reason = "cause"})
e2 = box.error.new({code = 111, reason = "err"})
e2:set_prev(e1)
-- There's no means to set e2 to diagnostic area.

This patch makes box.error() always promote error to the diagnostic
area despite of passed arguments.

Closes #4829

@TarantoolBot document
Title: always promote error created via box.error() to diag

box.error() is able to accept two types of argument: either pair of code
+ reason (box.error{code = 555, reason = 'Arbitrary message'}) or error
object (box.error(err)). In the first case error is promoted to
diagnostic area, meanwhile in the latter - it is not:
```
e1 = box.error.new({code = 111, reason = "cause"})
box.error({code = 111, reason = "err"})
- error: err
box.error.last()
- err
box.error(e1)
- error: cause
box.error.last()
- err
```
From now box.error(e1) sets error to diagnostic area as well:
```
box.error(e1)
- error: cause
box.error.last()
- cause
```
---
Patch is located on branch with stacked diagnostics since patch
is required to write correct tests for it.

Branch: https://github.com/tarantool/tarantool/commit/cb1010c1497f1d134e576b5bbbd6c1d0520ff7f4
Issue: https://github.com/tarantool/tarantool/issues/4829

 src/box/lua/error.cc    | 13 ++++++++++---
 test/box/error.result   | 22 ++++++++++++++++++++++
 test/box/error.test.lua |  8 ++++++++
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 08c2d983d..b2625bf5f 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -114,9 +114,16 @@ luaT_error_call(lua_State *L)
 			return luaT_error(L);
 		return 0;
 	}
-	if (lua_gettop(L) == 2 && luaL_iserror(L, 2))
-		return lua_error(L);
-	struct error *e = luaT_error_create(L, 2);
+	struct error *e = NULL;
+	if (lua_gettop(L) == 2) {
+		e = luaL_iserror(L, 2);
+		if (e != NULL) {
+			/* Re-set error to diag area. */
+			diag_set_error(&fiber()->diag, e);
+			return lua_error(L);
+		}
+	}
+	e = luaT_error_create(L, 2);
 	if (e == NULL)
 		return luaL_error(L, "box.error(): bad arguments");
 	diag_set_error(&fiber()->diag, e);
diff --git a/test/box/error.result b/test/box/error.result
index 4f0f30491..9ab4d1f6d 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -786,6 +786,28 @@ assert(e1.prev == e2)
  | - true
  | ...
 
+-- gh-4829: always promote error created via box.error() to
+-- diagnostic area.
+e1 = box.error.new({code = 111, reason = "cause"})
+ | ---
+ | ...
+box.error({code = 111, reason = "err"})
+ | ---
+ | - error: err
+ | ...
+box.error.last()
+ | ---
+ | - err
+ | ...
+box.error(e1)
+ | ---
+ | - error: cause
+ | ...
+assert(box.error.last() == e1)
+ | ---
+ | - true
+ | ...
+
 space:drop()
  | ---
  | ...
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index 66a22db90..c4783031e 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -213,4 +213,12 @@ box.error.set(e1)
 box.error.clear()
 assert(e1.prev == e2)
 
+-- gh-4829: always promote error created via box.error() to
+-- diagnostic area.
+e1 = box.error.new({code = 111, reason = "cause"})
+box.error({code = 111, reason = "err"})
+box.error.last()
+box.error(e1)
+assert(box.error.last() == e1)
+
 space:drop()
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] box: always promote error created via box.error() to diag

[Vladislav Shpilevoy]

Thanks for the patch!

On 01/04/2020 17:52, Nikita Pettik wrote:
> Note that it is vital that box.error() now promotes error to diag, since
> otherwise user is unable to set to diag custom error, which in turn is

Why unable? I thought we have box.error.set(). You can always do

    box.error.set(error_object)
    box.error()

This is equivalent of

    box.error(error_object)

which you are doing in this patch.

> linked with other errors:
> 
> e1 = box.error.new({code = 111, reason = "cause"})
> e2 = box.error.new({code = 111, reason = "err"})
> e2:set_prev(e1)
> -- There's no means to set e2 to diagnostic area.

tarantool> e1 = box.error.new({code = 111})
---
...

tarantool> e2 = box.error.new({code = 111})
---
...

tarantool> e2:set_prev(e1)
---
...

tarantool> box.error.set(e2)
---
...

tarantool> box.error.last() == e2
---
- true
...

> This patch makes box.error() always promote error to the diagnostic
> area despite of passed arguments.

But I am not against this patch. It is a nice sugar, when you need to
both set + throw. It is LGTM except the commit message due to comments
above.

> Closes #4829
> 
> @TarantoolBot document
> Title: always promote error created via box.error() to diag
> 
> box.error() is able to accept two types of argument: either pair of code
> + reason (box.error{code = 555, reason = 'Arbitrary message'}) or error
> object (box.error(err)). In the first case error is promoted to
> diagnostic area, meanwhile in the latter - it is not:
> ```
> e1 = box.error.new({code = 111, reason = "cause"})
> box.error({code = 111, reason = "err"})
> - error: err
> box.error.last()
> - err
> box.error(e1)
> - error: cause
> box.error.last()
> - err
> ```
> From now box.error(e1) sets error to diagnostic area as well:
> ```
> box.error(e1)
> - error: cause
> box.error.last()
> - cause
> ```
> ---

Re: [Tarantool-patches] [PATCH] box: always promote error created via box.error() to diag

[Nikita Pettik]

On 02 Apr 02:37, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 01/04/2020 17:52, Nikita Pettik wrote:
> > Note that it is vital that box.error() now promotes error to diag, since
> > otherwise user is unable to set to diag custom error, which in turn is
> 
> Why unable? I thought we have box.error.set(). You can always do
> 
>     box.error.set(error_object)
>     box.error()

Lol. It was me who introduced box.error.set() a week ago and I've
already forgotten about it. *Facepalm*

Dropped corresponding paragraph from commit message.

Also @ChangeLog (2.4):

* box.error() now can accept error object as an argument. Behaviour
is the same as for 'code + reason' pair of arguments: error is set
to Tarantool's diagnostic area and is thrown.

> This is equivalent of
> 
>     box.error(error_object)
> 
> which you are doing in this patch.
> 
> But I am not against this patch. It is a nice sugar, when you need to
> both set + throw. It is LGTM except the commit message due to comments
> above.
> 
> > Closes #4829
> > 
> > @TarantoolBot document
> > Title: always promote error created via box.error() to diag
> > 
> > box.error() is able to accept two types of argument: either pair of code
> > + reason (box.error{code = 555, reason = 'Arbitrary message'}) or error
> > object (box.error(err)). In the first case error is promoted to
> > diagnostic area, meanwhile in the latter - it is not:
> > ```
> > e1 = box.error.new({code = 111, reason = "cause"})
> > box.error({code = 111, reason = "err"})
> > - error: err
> > box.error.last()
> > - err
> > box.error(e1)
> > - error: cause
> > box.error.last()
> > - err
> > ```
> > From now box.error(e1) sets error to diagnostic area as well:
> > ```
> > box.error(e1)
> > - error: cause
> > box.error.last()
> > - cause
> > ```
> > ---

Re: [Tarantool-patches] [PATCH] box: always promote error created via box.error() to diag

[Vladislav Shpilevoy]

Thanks for the fixes!

> commit 28c43bbdb82f78b7a2d19e55de181ddb7d370497
> Author: Nikita Pettik <korablev@tarantool.org>
> Date:   Wed Apr 1 16:32:22 2020 +0300
> 
>     box: always promote error created via box.error() to diag
>     
>     This patch makes box.error() always promote error to the diagnostic
>     area despite of passed arguments.
>     
>     Closes #4829
>     
>     @TarantoolBot document
>     Title: always promote error created via box.error() to diag
>     
>     box.error() is able to accept two types of argument: either pair of code
>     + reason (box.error{code = 555, reason = 'Arbitrary message'}) or error

This '+' in the beginning of a line turns into a bullet point in
github markdown. Check 'Preview' when try to create a ticket. You
can escape it using \+, or move 'code' word on the same line
before +.

Re: [Tarantool-patches] [PATCH] box: always promote error created via box.error() to diag

[Nikita Pettik]

On 03 Apr 00:20, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > commit 28c43bbdb82f78b7a2d19e55de181ddb7d370497
> > Author: Nikita Pettik <korablev@tarantool.org>
> > Date:   Wed Apr 1 16:32:22 2020 +0300
> > 
> >     box: always promote error created via box.error() to diag
> >     
> >     This patch makes box.error() always promote error to the diagnostic
> >     area despite of passed arguments.
> >     
> >     Closes #4829
> >     
> >     @TarantoolBot document
> >     Title: always promote error created via box.error() to diag
> >     
> >     box.error() is able to accept two types of argument: either pair of code
> >     + reason (box.error{code = 555, reason = 'Arbitrary message'}) or error
> 
> This '+' in the beginning of a line turns into a bullet point in
> github markdown. Check 'Preview' when try to create a ticket. You
> can escape it using \+, or move 'code' word on the same line
> before +.
>

Fixed.