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

Proposed mods to callWithHandler #179

Closed

Conversation

rpatters1
Copy link
Contributor

@rpatters1 rpatters1 commented Mar 12, 2024

This is a draft PR for discussion purposes. I am trying to address 2 additional issues that I raised in (now closed) #120.

  • Null error handler. It would be nice if the code could skip the error handler if it is nullptr. I propose a modification in this PR, but for some reason it doesn't work. When there is an an error, the error code returned by lua_pcall is 5 ("error in error handling") instead of the actual error. I cannot find any difference in the code path from call vs. calling callWithHandler with a null handler. But Lua is obviously seeing one. I'd appreciate your ideas on why this might be.
  • Always raise LuaException. There are two types of error handlers: those that monitor and propagate and those that fix/address. It seems to me that it should be quite easy for lua_pcall to know which is which, because if the error is handled, it shouldn't return an error status. If the error is not handled, then the if block should happen. So there is no need for the conditional. It should always raise the error in the if block. This modification is working for me.

Also, a final question. When the error handler is not null, this code is working as I would like it to. My question is if there is a need to clean up the stack that has the error handler pushed on it.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Mar 12, 2024

I figured out why the null check wasn't working and have now applied a fix. So this PR addresses both of my issues now.

@rpatters1
Copy link
Contributor Author

I also think callWithHandler needs a static_assert to guarantee that the handler is a Lua CFunction, but my late-model c++ chops are not quite up to the task. I can get it to work for function pointers, but the static_assert throws on rvalue functions.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Mar 17, 2024

It turns out there is no way to suppress the error return from lua_pcall. The error handler does not seem to be able to do it. In this I was mistaken. So the question becomes, how do we handle the situation where the error handler propagates the error and when it doesn't?

If we aren't going to throw when there is an error handler present (my preference is that we do throw), then I need a way to create a LuaException from the LuaResult, so that I can throw it myself.

@rpatters1
Copy link
Contributor Author

I have fixed issues that broke the tests and added tests for std::function and null-valued lua_CFunction handlers. The question remains open:

  • Should it throw when there is an error handler (and an error occurred)?

Marking ready for review.

@rpatters1 rpatters1 marked this pull request as ready for review March 17, 2024 03:35
@kunitoki
Copy link
Owner

kunitoki commented Mar 17, 2024

When you use the error handler, the invocation should never throw, it's responsibility of the handler to decide what to do with the error and throw or not depending on the use case. If we always throw, there is no way to call a method without raising exceptions.

@kunitoki
Copy link
Owner

Improved in #180

@rpatters1
Copy link
Contributor Author

Something else I just discovered is that the error message gets suppressed if the handler is a std::function. This is due to the fact that the std::function gets called at CFunction.h line 793. This line pushes the return value from the error handler onto the Lua stack and thereby hides the error message.

I was going to add to the test to guarantee that the error message was returned in the LuaResult, but I see you've started your own PR for this. So I'll just flag it here for now.

@kunitoki
Copy link
Owner

Well the error handler should not return anything for luabridge to recover correctly the error message

@rpatters1
Copy link
Contributor Author

The error handler is int-valued. It has to return an integer.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Mar 17, 2024

When you use the error handler, the invocation should never throw, it's responsibility of the handler to decide what to do with the error

That's fair, but understand that there is no way to suppress the error return from lua_pcall (unless I throw directly out of the error handler, which is dangerous because it is called by Lua, not LuaBridge). That means ultimately I have to handle the error where I call callWithHandler.

This will will work okay if I can construct a LuaException from the LuaResult. I got this to work on one of my branches, which I will apply to #180 for your consideration.

@kunitoki
Copy link
Owner

If errfunc is 0, then the [error](https://pgl.yoyo.org/luai/i/error) message returned on the stack is exactly the original [error](https://pgl.yoyo.org/luai/i/error) message. Otherwise, errfunc is the stack index of an [error](https://pgl.yoyo.org/luai/i/error) handler function. (In the current implementation, this index cannot be a pseudo-index.) In case of runtime [error](https://pgl.yoyo.org/luai/i/error)s, this function will be called with the error message and its return value will be the message returned on the stack by [lua_pcall](https://pgl.yoyo.org/luai/i/lua_pcall).

Typically, the [error](https://pgl.yoyo.org/luai/i/error) handler function is used to add more debug information to the [error](https://pgl.yoyo.org/luai/i/error) message, such as a stack traceback. Such information cannot be gathered after the return of [lua_pcall](https://pgl.yoyo.org/luai/i/lua_pcall), since by then the stack has unwound

So in theory, the returned value of the error handler should be the one that will be seen by the pcall. When using a std::function, it should return a std::string, and when is a lua_CFunction it must push a string on the stack and return 1.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Mar 17, 2024

Typically, the error handler function is used to add more debug information to the error message, such as a stack traceback. Such information cannot be gathered after the return of lua_pcall, since by then the stack has unwound

This is exactly the way I use it, which is why I would have preferred a throw upon return from lua_pcall. The error function does not handle the error. It just gets information about it. I can deal with this, though.

In case of runtime errors, this function will be called with the error message and its return value will be the message returned on the stack by lua_pcall.

I think this may be misleading, just tracing the Lua code. Yes, the error func is called with the error message on the top of the Lua stack, but the message is also returned on the Lua stack (not the return value of the C function.) Without modifying the stack at all, I've tried returning both 0 and 1 from error functions and seen no difference in behavior. I'm guessing no positive value makes a difference, but I haven't tried it.

The Lua code expects the error function always to return an integer. It is possible for the error function to add a new or additional message on the Lua stack, but the error function itself should always be a Lua CFunction. I don't plan to use std::function for my own error handler, but it strikes me as very odd that all other ways of specifying error handlers (functions, lamdas, and pointers) are Lua CFunctions, but mysteriously std::function should return a string. This seems wrong to me. At the very least it will require detailed documentation to explain the discrepancy.

@rpatters1
Copy link
Contributor Author

closing, since #180 takes care of it.

@rpatters1 rpatters1 closed this Mar 18, 2024
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.

2 participants