Standardize Error Handling in Builtin Programs
fatboychummy opened this issue ยท 7 comments
I am currently discussing with Xella the implementation of a feature to detect failed installations for PineStore download tracking. Specifically, if an installation fails, it should not log a download. However, we've encountered an issue with many built-in programs (such as pastebin get
/run
) not returning an error state directly through error()
, but instead using printError()
.
This poses a challenge as there is no clear indication of installation failure when tracking the program's status. Consequently, since the program consistently returns an OK status, determining if the installation actually failed becomes problematic.
Thus, I would like to go over all builtin CC programs and change any printError
's/io.stderr:write()
's into proper error()
s. I am, however, not entirely sure how this may affect things overall, so I would appreciate any insights, suggestions, or alternative solutions.
I am not sure of a timeframe on this as well, as I'm running into finals, but I would like to look into this sometime in the next month.
Yeah, the lack of a good os.exit(1)
alternative is definitely a pain. The trouble with error
is there's no good way to tell if something is user-friendly error (where we just want to print it) vs a bug (where we might want to print the context or a stack trace). CC currently does this by checking the error contains a filename and line number, but other shells like mbs don't bother.
The "best" pattern here is probably to display the message normally (printError
/io.stderr:write
), and then exit with a no-args error()
.
Edit: Though lua
and edit
's run wrapper don't handle that very well. Technically the most standard-compliant option is to do error(setmetatable({}, {__tostring = function() return "My error message" end }))
, but that's even uglier! We could have a helper for this I guess??? Not sure!
As an aside, installers really shouldn't be shelling out to pastebin/wget
. Those programs make sense as the entrypoint to an installer, but anything else should just use the http
API normally!
As an aside, installers really shouldn't be shelling out to pastebin/wget. Those programs make sense as the entrypoint to an installer, but anything else should just use the http API normally!
How come?
Yeah, the lack of a good
os.exit(1)
alternative is definitely a pain. The trouble witherror
is there's no good way to tell if something is user-friendly error (where we just want to print it) vs a bug (where we might want to print the context or a stack trace). CC currently does this by checking the error contains a filename and line number, but other shells like mbs don't bother.The "best" pattern here is probably to display the message normally (
printError
/io.stderr:write
), and then exit with a no-argserror()
.Edit: Though
lua
andedit
's run wrapper don't handle that very well. Technically the most standard-compliant option is to doerror(setmetatable({}, {__tostring = function() return "My error message" end }))
, but that's even uglier! We could have a helper for this I guess??? Not sure!As an aside, installers really shouldn't be shelling out to
pastebin/wget
. Those programs make sense as the entrypoint to an installer, but anything else should just use thehttp
API normally!
You can just do error("My error message", 0)
, that will lead same result than printError
if no one catched it
Finally looking like I have time to work on this a bit.
Edit: Though
lua
andedit
's run wrapper don't handle that very well. Technically the most standard-compliant option is to doerror(setmetatable({}, {__tostring = function() return "My error message" end }))
, but that's even uglier! We could have a helper for this I guess??? Not sure!
The helper should (hopefully) be easy enough to do, but I'm more curious about what we would want for data in the error object, if we went this route. My initial thoughts would be have a type
field with specific defined "types", and each type would then have a set of defined fields. For some rough examples:
-- Instead of
if err == "Too long without yielding" then
-- Use
if err.type == "timeout" then -- or something along these lines
-- Instead of
if err:match("attempt to use a closed file") then
-- Use
if err.type == "closed_file" then
print("Attempted to use closed filehandle of file", err.filename)
-- And for a more extreme example...
-- Instead of
local line, argn, expected, got = err:match(":(%d+): Bad argument #(%d+): Expected (%S+), got (%S+)")
if line then
print("Bad argument:", line, argn, expected, got)
-- ...
-- Use
if err.type == "argument_type_mismatch" then
print("Bad argument:", err.line, err.argn, err.expected, err.got)
-- ...
I'm blanking on other things that would be useful to have available in every such object, other than possibly line number and epoch time? Perhaps just stuff from debug.getinfo
?
Do we also want to override error
so that it converts error("some message")
into an error object with some simple data, say:
error("some message")
-> setmetatable(
{
type = "message",
rawMessage = "some message",
currentline = 71,
short_src = "/error_test.lua",
-- ...
},
{__tostring = function() return "/error_test.lua:71: some message" end}
)
Do we want errors raised in java methods to have this object structure?
Edit: Woops, forgot to rewrite part of the first paragraph before I sent this.
As an aside, installers really shouldn't be shelling out to pastebin/wget. Those programs make sense as the entrypoint to an installer, but anything else should just use the http API normally!
How come?
Ideally you should handle the requests/downloads yourself for more control over whether or not there was an issue downloading your software.
PineStore's issue (and the main reason I noticed that some programs don't actually throw errors) is that it is less an install script, and more an install command fetcher and download logger, if that makes sense. It's not actually the script that is installing a user's program, instead it wraps around a command. I think I explained this part poorly in the original issue body, so I'll give a better explanation below.
Explanation
In PineStore, you can set an install command. The script PineStore uses acts as a wrapper around this command, and it pretty much just does something along the lines of if shell.run(install_command) then log_download() else error("Installation failed!") end
.
However, wget
never explicitly calls error
(instead just calling printError
). Thus, if your install command is something like wget run https://example-site.com/installer.lua
and example-site.com
is down for whatever reason, the following events will happen:
- The wrapper will run the install command.
wget run https://example-site.com/installer.lua
will fail, since the server is down/unreachable. No error is raised, however.shell.run
returns an OK status -- the install command didn't error.- The PineStore wrapper erroneously logs a successful download.
I think it's useful to split errors into two categories:
- Runtime errors: these are errors that occur due to bugs in the program (e.g.
Too long without yielding
,attempt to index nil
, etc...). These (typically) have a source position associated with them, and errors are pretty printed in the shell. - User-facing errors: This is what you mention in the original issue. These are errors intentionally thrown by programs to indicate a problem to the user.
There's definitely some overlap here, but I think it's probably a mistake to mix the two up. I think we should probably just focus on user-facing errors for now, as that's much simpler.
As far as structured runtime errors goes, it's a bit of a backwards compatibility nightmare.
CC does have an (undocumented) notion of "rich" errors, which are errors that capture the context where they were thrown. This allows coroutine managers to rethrow errors and have the shell report the error in a fancy way (see an example in metis).
In #1320, I originally had parallel
use this rich error system, but that broke several programs doing pcall(parallel.waitForAny)
and pattern-matching on the error. This is terrible code, but unfortunately we have seen it in the wild!
Changing the actual Lua VM to throw structured errors is a definite no-no. I think we could improve error reporting by matching on the error and maybe parsing the code (see #1245 (comment)), but this isn't something that needs/should be exposed to the user.
I originally had
parallel
use this rich error system, but that broke several programs doingpcall(parallel.waitForAny)
and pattern-matching on the error. This is terrible code, but unfortunately we have seen it in the wild!
I'll admit this is really ugly looking, but we could do something like this to still support pattern matching (and other string operations) on these rich error objects:
local rich_error = setmetatable(
{
message = "Match test 1-2-3",
},
{
__tostring = function(self)
return self.message
end,
__index = function(_, idx)
if string[idx] then
if type(string[idx]) == "function" then
return function(self, ...)
return string[idx](tostring(self), ...)
end
end
return string[idx]
end
end,
__concat = function(a, b)
return tostring(a) .. tostring(b)
end,
}
)
print(rich_error:match("Match test"))
print(rich_error .. " noot")
Thoughts? If I went through with this and wrote such a rich error builder module, should it be put in as a cc.internal
module, or expose it as something like cc.errors
?
If you think this will lead to too many issues, I'll probably just go ahead and start doing what was originally suggested -- printing the error, then no-arg erroring. Though that brings up another question: In programs which do call error
instead of printError
, should these also be updated to call printError(message); error()
?