Re: [Tarantool-patches] [PATCH luajit v2 1/2] Print errors from __gc finalizers instead of rethrowing them.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maksim!
Thanks for the fixes!
LGTM, just some minor nits below.

On 08.11.23, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Finalizers are not supposed to throw errors -- this is undefined behavior.
> Lua 5.1 - 5.3 and (previously) LuaJIT rethrow the error. This randomly
> breaks some unrelated code that just happens to do an allocation. Bad.
> Lua 5.4 catches the error and emits a warning instead. But warnings are
> not enabled by default, so it fails silently. Even worse.
> LuaJIT (now) catches the error and emits a VM event. The default event
> handler function prints "ERROR in finalizer: ...".
> Set a custom handler function with: jit.attach(handler, "errfin")
> 
> (cherry-picked from commit 1c279127050e86e99970100e9c42e0f09cd54ab7)
> 
> The default handler for finalizer errors is set during the
> Lua initialization. Namely, in the `luaL_newstate`.
> 
> Along with the introduction of the new `ERRFIN` VM event, the high
> bits for the old VM events are removed since they are scratched
> anyway by the bitwise operation `(hash)<<3` in the `VMEVENT_DEF`
> macro.
> 
> This patch results in a regression in the PUC-Rio test suite. The
> test in the suite for the error in the GC finalizer fails after
> the patch because the error is now handled with the VM event
> handler instead of being rethrown. Hence, the `collectgarbage`
> finishes successfully despite the error in the GC finalizer.
> Considering this change, the test was disabled.
> 
> There is also another regression in the `misclib-getmetrics-capi`,
> because there are a few test cases reliant on the `lua_gettop(L)`
> value, which is broken after this patch. The `_VMEVENTS` table,
> where the error handler for GC finalizers is set, was not cleared
> from the stack after the initialization. This issue is fixed in
> the following patch.
> 
> Maxim Kokryashkin:
> * added the test for the problem and the description for the patch
> 
> Part of tarantool/tarantool#9145
> ---
> Q:
> >> +-- The test below is disabled for LuaJIT, since it handles errors from
> >> +-- GC finalizers via VM event.
> >> -- errors during collection
> >> -u = newproxy(true)
> >> -getmetatable(u).__gc = function () error "!!!" end
> >> -u = nil
> >> -assert(not pcall(collectgarbage))
> >> -
> >> +-- u = newproxy(true)
> >> +-- getmetatable(u).__gc = function () error "!!!" end
> >> +-- u = nil
> >> +-- assert(not pcall(collectgarbage))
> >>
> >
> >Maybe its better just to setup the "errfin" handler to `error()` function?
> 
> A: Well, that is not going to fix the test case anyway. See, the
> `lj_vmevent_call` is a protected call and any errros which happened
> during the vmevent handling are silently printed into the stderr. So
> the collectgarbage call here won't fail even in this case.

OK, lets left it as is.

> 
> Q:
> >> +local function errfin_handler()
> >> + error_in_finalizer = true
> >> +end
> >
> >Is it better just to add `test:ok(true, 'error handler called')` here?
> 
> A: Nope, because I want to test not only that there is no error, but
> also that the finalizer error handler was called.

So, the test will be passed, if the `errfin_handler()` is called.
If it isn't, we got bad plan error, so test fails.
As a bonus, we check that handler is called only once for each error :).

> 
> 
> Q: I suggest to test the default handler too, within the separate test.
> 
> Also, maybe we should test other cases (error function (in default
> case), function with tailcall to error, etc.), any ideas about them?
> 
> A: I've added a test for the default handler. I doubt that testing
> for other cases you have mentioned is meaningful, because all of
> these errors are going to be silently printed into the stderr.

OK, thanks!

> 
>  src/Makefile.dep.original                     | 14 +++----
>  src/lib_aux.c                                 | 33 ++++++++++++++-
>  src/lj_gc.c                                   | 10 ++++-
>  src/lj_vmevent.h                              |  7 ++--
>  test/PUC-Rio-Lua-5.1-tests/gc.lua             | 12 +++---
>  ...6-print-errors-from-gc-fin-custom.test.lua | 42 +++++++++++++++++++
>  ...-print-errors-from-gc-fin-default.test.lua | 11 +++++
>  .../script.lua                                | 24 +++++++++++
>  8 files changed, 133 insertions(+), 20 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua
>  create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua
>  create mode 100644 test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua
> new file mode 100644
> index 00000000..71efc260
> --- /dev/null
> +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-custom.test.lua
> @@ -0,0 +1,42 @@
> +local tap = require('tap')
> +local test = tap.test('lj-946-print-errors-from-gc-fin-custom'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +local ffi = require('ffi')
> +local error_in_finalizer = false
> +
> +local function errfin_handler()
> +    error_in_finalizer = true
> +end
> +
> +local function new_bad_cdata()
> +  return ffi.gc(ffi.new('char [?]', 1024), 'uncallable string')
> +end
> +
> +local function test_f()
> +  collectgarbage('collect')
> +  -- Make GC aggressive enough to end the atomic phase before
> +  -- exiting the trace.
> +  collectgarbage('setstepmul', 400)
> +  -- The number of iterations is empirical, just big enough for the

Nit: comment line length is more than 66 symbols.

> +  -- issue to strike.
> +  for _ = 1, 4000 do
> +    new_bad_cdata()
> +  end
> +end
> +
> +jit.opt.start('hotloop=1')
> +-- Handler is registered but never called before the patch.
> +-- It should be called after the patch.
> +jit.attach(errfin_handler, 'errfin')
> +local status = pcall(test_f)
> +-- We have to stop GC now because any step raises the error due to
> +-- cursed cdata objects.
> +collectgarbage('stop')
> +test:ok(status, 'test function completed successfully')
> +test:ok(error_in_finalizer, 'error handler called')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua
> new file mode 100644
> index 00000000..dfef11e5
> --- /dev/null
> +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default.test.lua
> @@ -0,0 +1,11 @@
> +local tap = require('tap')
> +local test = tap.test('lj-flush-on-trace'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local script = require('utils').exec.makecmd(arg, { redirect = '2>&1' })
> +local output = script()
> +test:like(output, '.*ERROR in finalizer:.*')

Minor: '.*' aren't necessary here. I suppose, that regex
'ERROR in finalizer:' is much readable and has the same meaning.

> +test:done(true)
> diff --git a/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua
> new file mode 100644
> index 00000000..fdd9ced1
> --- /dev/null
> +++ b/test/tarantool-tests/lj-946-print-errors-from-gc-fin-default/script.lua
> @@ -0,0 +1,24 @@
> +local ffi = require('ffi')
> +
> +local function new_bad_cdata()
> +  return ffi.gc(ffi.new('char [?]', 1024), 'uncallable string')
> +end
> +
> +local function test_f()
> +  collectgarbage('collect')
> +  -- Make GC aggressive enough to end the atomic phase before
> +  -- exiting the trace.
> +  collectgarbage('setstepmul', 400)
> +  -- The number of iterations is empirical, just big enough for the

Nit: comment line length is more than 66 symbols.

> +  -- issue to strike.
> +  for _ = 1, 4000 do
> +    new_bad_cdata()
> +  end
> +end
> +
> +jit.opt.start('hotloop=1')
> +local status = pcall(test_f)
> +-- We have to stop GC now because any step raises the error due to
> +-- cursed cdata objects.
> +collectgarbage('stop')
> +assert(status, 'error is not rethrown')
> --
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
Sergey Kaplun