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] Clean up uncaught error handling #11082

Merged
merged 6 commits into from
Apr 4, 2023

Conversation

tobil4sk
Copy link
Member

@tobil4sk tobil4sk commented Apr 3, 2023

Alternative solution to #10979, without needing a new flag.

Removes usage of os.exit, which makes it safer and applicable both
to lua scripts embedded in another application and standalone scripts.
This removes the need for the lua-standalone flag.
If it is dce'd, we get a mess when lua prints an uncaught error
@sebthom
Copy link
Contributor

sebthom commented Apr 3, 2023

The reason for the new flag was to keep the current default behaviour that the Lua process is not exited if an error occurs (in case the lua script is executed in an embedded lua interpreter as a plugin). And if one enables the flag the lua process is exited with an error code. I don't see that this is addressed in that PR, it looks more like it reverts back to the old behaviour.

@tobil4sk
Copy link
Member Author

tobil4sk commented Apr 3, 2023

I don't see that this is addressed in that PR, it looks more like it reverts back to the old behaviour.

The behaviour is mostly the same as the previous PR, except that the correct exit code achieved without manually calling os.exit. This means the same code is safe to use for both embedded scripts and standalone scripts, so no need for the flag.

With the version in the development branch, without -D lua-standalone defined, it would simply not catch the error at all, and let it bubble up to the program running the script. In this version, it catches the error and rethrows it, so the end result is the same for embedded lua scripts (i.e. the program running the script has to deal with the error).

With -D lua-standalone defined in the dev branch, it would print out the error before running a manual os.exit. With this version, it catches the error, constructs the useful error message with the call stack, and rethrows it again so lua can take care of printing it/exiting with the correct error code. So the error is still printed with the callstack and everything, and the exit code is correct, without having to manually use os.exit.

So in both cases the end result is the same, but the reason to have the behaviour locked behind a flag, that being os.exit, has been avoided. The behaviour is now portable, so a Haxe generated lua script can be reused as a script embedded environment and as a standalone script, while the error handling is still as expected for both.

@kLabz
Copy link
Contributor

kLabz commented Apr 3, 2023

This will need tests demonstrating the different behaviors

@tobil4sk
Copy link
Member Author

tobil4sk commented Apr 3, 2023

For reference, here is an example of the code that would generate with this PR:

function _hx_handle_error(obj)
  -- construct a useful error with a call stack
  local message = tostring(obj)
  if _G.debug and _G.debug.traceback then
    -- level 2 to skip _hx_handle_error
    message = _G.debug.traceback(message, 2)
  end
  -- __tostring method allows it to be printed by lua
  return setmetatable({}, { __tostring = function() return message end })
end

_hx_static_init();
local success, err = _G.xpcall(function() 
  ___Main_Main_Fields_.main();
  _hx_luv.run();
end, _hx_handle_error)
-- if there was an uncaught error, rethrow it
if not success then
  _G.error(err)
end

Lua 5.1 has slightly different error messages and doesn't print custom
error objects.
@tobil4sk
Copy link
Member Author

tobil4sk commented Apr 3, 2023

Updated with test cases, which now make sure:

  • Standalone scripts exit with 1 in case of an uncaught error
  • Standalone scripts with uncaught errors print the exception value and stacktrace (where possible, i.e. 5.2+)
  • An embedded script that has an error does not immediately quit the host program (in this case, another lua script)
  • The host program is able to handle the error itself

Tests cover both uncaught Haxe exceptions and uncaught native exceptions, e.g. "attempt to index a nil value".

@kLabz kLabz added this to the Release 4.3 milestone Apr 4, 2023
@kLabz
Copy link
Contributor

kLabz commented Apr 4, 2023

@sebthom does that seem fine to you? I'd rather merge this before 4.3 if we're going this route; wouldn't make much sense to introduce a flag in 4.3 and remove it right after.

@sebthom
Copy link
Contributor

sebthom commented Apr 4, 2023

that's fine with me.

@kLabz
Copy link
Contributor

kLabz commented Apr 4, 2023

Let's go then! Thanks :)

@kLabz kLabz merged commit a6aee34 into HaxeFoundation:development Apr 4, 2023
@tobil4sk tobil4sk deleted the lua-errors branch April 4, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants