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

Propagate exceptions from parallel where possible #2095

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

SquidDev
Copy link
Member

@SquidDev SquidDev commented Feb 9, 2025

In the original implementation of our prettier runtime errors (#1320), we wrapped the errors thrown within parallel functions into an exception object. This means the call-stack is available to the catching-code, and so is able to report a pretty exception message.

Unfortunately, this was a breaking change, and so we had to roll that back. Some people were pcalling the parallel function, and matching on the result of the error. For example:

local ok, result = pcall(parallel.waitForAny, f, g)
if not ok and result == "Terminated" then
  -- ...
else
  -- ...
end

This is a second attempt at this, using a technique I've affectionately dubbed "magic throws". The parallel API is now aware of whether it is being pcalled or not, and thus able to decide whether to wrap the error into an exception or not:

An example of calling parallel.waitForAll with a function that calls error. In the first case, the waitForAll is called directly — the error is reported via CC's error printing mechanism. In the second case, the waitForAll is pcalled, and the error is just a raw string.

I think there are some risks with this approach. It's not obvious to users whether you'll get the fancy error reporting or not, and that could be confusing — the "magic" in "magic throws" is not a necessarily a good thing!

I still think this change is worth merging (we can always revert it if it proves to be a mistake), but definitely open to feedback.

I'd quite like to extend this to approach to other bits of the code — we could do something similar to provide better error messages for require.

@SquidDev SquidDev added enhancement An extension of a feature or a new feature. feedback wanted Tell me what you want, what you really really want. area-CraftOS This affects CraftOS, or any other Lua portions of the mod. labels Feb 9, 2025
@SquidDev SquidDev merged commit 051c70a into mc-1.20.x Feb 13, 2025
6 checks passed
@SquidDev SquidDev deleted the feature/magic-throws branch February 13, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CraftOS This affects CraftOS, or any other Lua portions of the mod. enhancement An extension of a feature or a new feature. feedback wanted Tell me what you want, what you really really want.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant