Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lua: set error trace to the caller place #9996

Merged
merged 5 commits into from
Jun 6, 2024

Conversation

nshy
Copy link
Contributor

@nshy nshy commented May 8, 2024

Currently trace points to the C code where diag is set or Lua code where error is thrown. So it points to internal code which is not particularly useful for the user. Let's make trace point the place where API is called. This patch do it for API residing in several modules only though (schema.lua is one of them).

Closes #9914

DO NOT MERGE Depends on PR tarantool/test-run#428

See EE PR https://github.com/tarantool/tarantool-ee/pull/791

@nshy nshy requested review from Totktonada and a team as code owners May 8, 2024 13:49
@nshy nshy added the do not merge Not ready to be merged label May 8, 2024
@coveralls
Copy link

coveralls commented May 8, 2024

Coverage Status

coverage: 87.091% (-0.03%) from 87.125%
when pulling cca9139 on nshy:error-add-lua-trace
into 1f75231
on tarantool:master
.

@nshy nshy force-pushed the error-add-lua-trace branch 2 times, most recently from e98ddc4 to ff4bcba Compare May 13, 2024 08:58
@nshy nshy requested a review from locker May 13, 2024 09:35
@nshy nshy assigned nshy and unassigned locker May 13, 2024
@nshy nshy assigned locker and unassigned nshy May 14, 2024
Copy link
Member

@locker locker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through patches 1-3.

src/box/lua/index.c Outdated Show resolved Hide resolved
src/box/lua/schema.lua Show resolved Hide resolved
src/box/lua/console.lua Outdated Show resolved Hide resolved
src/box/lua/console.lua Outdated Show resolved Hide resolved
src/box/lua/console.lua Outdated Show resolved Hide resolved
src/box/lua/console.lua Outdated Show resolved Hide resolved
src/box/lua/console.lua Outdated Show resolved Hide resolved
@nshy
Copy link
Contributor Author

nshy commented May 15, 2024

Moved the last patch to the first place (as it fixes build error on my development environment).

src/box/lua/tuple.lua Outdated Show resolved Hide resolved
test/box/role.result Outdated Show resolved Hide resolved
src/box/lua/schema.lua Outdated Show resolved Hide resolved
@locker locker removed their assignment May 16, 2024
@nshy nshy assigned locker and unassigned nshy May 22, 2024
@nshy nshy changed the title schema: set error trace to the caller place lua: set error trace to the caller place May 22, 2024
src/box/lua/index.c Outdated Show resolved Hide resolved
src/box/lua/error.cc Outdated Show resolved Hide resolved
test/box-luatest/box_cfg_env_test.lua Show resolved Hide resolved
src/lua/utils.h Outdated Show resolved Hide resolved
Here is just of bunch utility functions that used in the patch that make
several modules throw box.error with trace set to the caller place. That
patch is just a boring huge switch to box.error and setting proper level
on error creation. Factor out utility functions so that they and their
tests are not get lost.

Part of tarantool#9914

NO_CHANGELOG=internal
NO_DOC=internal
@nshy nshy force-pushed the error-add-lua-trace branch 2 times, most recently from c8c62ef to 1c39bff Compare June 3, 2024 14:34
@nshy nshy assigned Totktonada and unassigned nshy Jun 4, 2024
@nshy nshy requested a review from Totktonada June 4, 2024 07:33
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK regarding the test/config-luatest changes.

@Totktonada Totktonada removed their assignment Jun 5, 2024
@nshy nshy added full-ci Enables all tests for a pull request and removed do not merge Not ready to be merged labels Jun 5, 2024
@nshy nshy self-assigned this Jun 5, 2024
@nshy
Copy link
Contributor Author

nshy commented Jun 5, 2024

Restored the original usage of return <raise_some_lua_error>() and unreachable() to fix clang compiler warnings.

diff
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 5a60a9d5a..2710dcc44 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -335,7 +335,7 @@ luaT_error_new(lua_State *L)
 	struct error *e = luaT_error_create(L, 1);
 	if (e == NULL) {
 		diag_set(IllegalParams, "box.error.new(): bad arguments");
-		luaT_error(L);
+		return luaT_error(L);
 	}
 	lua_settop(L, 0);
 	luaT_pusherror(L, e);
@@ -359,7 +359,7 @@ luaT_error_set(struct lua_State *L)
 {
 	if (lua_gettop(L) == 0) {
 		diag_set(IllegalParams, "Usage: box.error.set(error)");
-		luaT_error(L);
+		return luaT_error(L);
 	}
 	struct error *e = luaT_checkerror(L, 1);
 	diag_set_error(&fiber()->diag, e);
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index efcef24ee..307ba9dd7 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -758,7 +758,7 @@ lbox_backup_start(struct lua_State *L)
 		checkpoint_idx = luaT_checkint(L, 1);
 		if (checkpoint_idx < 0) {
 			diag_set(IllegalParams, "invalid checkpoint index");
-			luaT_error(L);
+			return luaT_error(L);
 		}
 	}
 	lua_newtable(L);
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 5821cab59..82a6fa9a9 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -945,6 +945,7 @@ lbox_fiber_slice_parse(struct lua_State *L, int idx)
 			 "slice must be a number or a table "
 			 "{warn = <number>, err = <number>}");
 		luaT_error(L);
+		unreachable();
 	}
 	if (!fiber_slice_is_valid(slice)) {
 		diag_set(IllegalParams, "slice value must be >= 0");
diff --git a/src/lua/init.c b/src/lua/init.c
index 9efc233be..c6787d39d 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -394,7 +394,7 @@ lbox_tonumber64(struct lua_State *L)
 		base = (base == -1 ? 10 : base);
 		if (base != 10) {
 			diag_set(IllegalParams, "argument 1 is not a string");
-			luaT_error(L);
+			return luaT_error(L);
 		}
 		lua_settop(L, 1); /* return original value as is */
 		return 1;
@@ -488,7 +488,7 @@ skip:		base = (base == -1 ? 10 : base);
 		base = (base == -1 ? 10 : base);
 		if (base != 10) {
 			diag_set(IllegalParams, "argument 1 is not a string");
-			luaT_error(L);
+			return luaT_error(L);
 		}
 		uint32_t ctypeid = 0;
 		luaL_checkcdata(L, 1, &ctypeid);

</details>

@nshy nshy removed the full-ci Enables all tests for a pull request label Jun 5, 2024
@nshy
Copy link
Contributor Author

nshy commented Jun 5, 2024

One of the integration test does not expect empty error trace (file is "" and line is 0) fox netbox eval. Fixed to keep existing error trace on luaL_loadbuffer() failure in `execute_lua_eval()

Also added a test for error trace on code evaluation and function call over netbox as suggested by @locker.

diff
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 51ce67997..5267e8777 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -447,7 +447,7 @@ execute_lua_eval(lua_State *L)
        uint32_t expr_len = ctx->name_len;
        if (luaL_loadbuffer(L, expr, expr_len, "=eval")) {
                diag_set(LuajitError, lua_tostring(L, -1));
-               luaT_error(L);
+               luaT_error_at(L, 0);
        }

        /* Unpack arguments */
diff --git a/test/box-luatest/error_subsystem_improvements_test.lua b/test/box-luatest/error_subsystem_improvements_test.lua
index 5379cd426..a81c25d4f 100644
--- a/test/box-luatest/error_subsystem_improvements_test.lua
+++ b/test/box-luatest/error_subsystem_improvements_test.lua
@@ -1051,6 +1051,40 @@ g.test_illegal_params = function()
     t.assert_equals(e.message, 'foo')
 end
 
+g.test_netbox_call_trace = function(cg)
+    local netbox = require('net.box')
+    local file = debug.getinfo(1, 'S').short_src
+    local line = debug.getinfo(1, 'l').currentline + 4
+    cg.server:exec(function()
+        box.schema.create_space('test')
+        rawset(_G, 'test_func', function()
+            box.space.test:insert({})
+        end)
+    end)
+
+    local function test_error(file, line, fn, ...)
+        local ok, err = pcall(fn, ...)
+        t.assert_not(ok)
+        t.assert(box.error.is(err))
+        t.assert_equals(err.message, "No index #0 is defined in space 'test'")
+        local trace = err.trace[1]
+        t.assert_equals(trace.line, line)
+        t.assert_equals(trace.file, file)
+    end
+
+    local c = netbox.connect(cg.server.net_box_uri)
+    test_error('eval', 3, c.eval, c, '\n\nbox.space.test:insert({})')
+    test_error(file, line, c.call, c, 'test_func')
+end
+
+g.after_test('test_netbox_call_trace', function(cg)
+    cg.server:exec(function()
+        if box.space.test ~= nil then
+            box.space.test:drop()
+        end
+    end)
+end)
+
 g.test_trace_checked = function()
    local st, res = pcall(t.assert_error,
                          tarantool._internal.raise_incorrect_trace)

nshy added 4 commits June 5, 2024 17:22
In scope of the tarantool#9914 issue we are setting error trace for API to the
caller frame. For the API written in LuaC without any Lua code around
it is easy task. Just make `luaT_error` set the proper trace.

By the way test we don't mess up trace for code evaluated thru netbox.

Part of tarantool#9914

NO_CHANGELOG=incomplete
NO_DOC=incomplete
Why these modules? Initially in the scope of tarantool#9914 we only want
to fix trace for `schema.lua` but there is an issue. In the next
patch we changing `console.lua` so that for existing diff test the
trace is checked (for specified modules).

In that patch we add a wrapper function around evaluated expression. So
that argument checking functions like `luaL_checklstring` start to refer
wrapper's ``fn`` in error instead of ``?``. We decided drop the usage of
such checkers in code covered by diff tests. Once we touch a module in
the scope this change we also fix all non box errors to box ones with
proper level.

Part of tarantool#9914

NO_CHANGELOG=incomplete
NO_DOC=incomplete
In scope of the tarantool#9914 issue we are setting error trace for API to the
caller frame. Let's leverage existing diff tests to check error trace.
Just check error trace when error is raised when evaluating expression
in console which is used in diff tests.

The check is done for test build only and only for Lua modules specified
in `tarantool._internal.trace_check_is_required`.

Part of tarantool#9914

NO_TEST=internal
NO_CHANGELOG=internal
NO_DOC=internal
Bump test-run to new version with the following improvements:

- Bump luatest to 1.0.1-14-gdfee2f3 [1]
- Adjust test result report width to terminal size [2]
- dispatcher: lift pipe buffer size restriction [3]
- flake8: fix E721 do not compare types [4]

[1] tarantool/test-run@84ebae5
[2] tarantool/test-run@1724211
[3] tarantool/test-run@81259c4
[4] tarantool/test-run@1037299

We also have to fix several tests that check that script with luatest
assertions have empty stderr output. test-run brings Luatest which
logs assertions at 'info' level.

Note that gh_8433_raft_is_candidate_test is different. Original
assertion involves logging huge tables that have somewhere closed
sockets inside. And 'socket.__tostring' currently raises error for
closed sockets.

We need to fix gh_6819_iproto_watch_not_implemented_test also to
account error created on loading `luatest` on `server:exec` after
the commit "Add trace check for error assertions".

Part of tarantool#9914

NO_DOC=internal
NO_TEST=internal
NO_CHANGELOG=internal
@nshy nshy added the full-ci Enables all tests for a pull request label Jun 5, 2024
@nshy nshy assigned locker and unassigned nshy Jun 6, 2024
@locker locker merged commit 97a801e into tarantool:master Jun 6, 2024
81 checks passed
@nshy nshy deleted the error-add-lua-trace branch June 6, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables all tests for a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Lua trace when throwing error raised in C to Lua
5 participants