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

When will panics become errors that can be handled? #113

Open
almarklein opened this issue Jun 15, 2021 · 9 comments
Open

When will panics become errors that can be handled? #113

almarklein opened this issue Jun 15, 2021 · 9 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@almarklein
Copy link
Collaborator

Whenever there is a validation error (e.g. a syntax error in the shader) Rust will panic, and bring down the whole process. This makes it hard to do more interactive things (e.g. in dynamic languages like Python). As I've understood it, the panics are a temporary thing and will eventually be replaced with an error mechanism, so that the calling environment can handle the error.

Are there ideas about what this will look like and when this will be happening?

@kvark
Copy link
Member

kvark commented Jun 16, 2021

This is already solved at wgpu-core level. All of the things that can error return Option<Error> on the side. It's up to the caller to figure out what to do with it. For example, wgpu-rs allows users to set the uncaptured error handler. The default handler does indeed panic.

So I think wgpu-native needs to expose the "on_uncaptured_error" callback, similarly.

@kvark kvark added enhancement New feature or request help wanted Extra attention is needed labels Jun 16, 2021
@sqaxomonophonen
Copy link
Contributor

I've begun work on improving the error handling in wgpu-native here:

sqaxomonophonen@5ebd50e

It's not yet finished, but it implements wgpuDeviceSetUncapturedErrorCallback(), and currently improves wgpuDeviceCreateShaderModule() and wgpuSwapChainGetCurrentTextureView() by not panic'ing when something goes wrong. Instead they call your error callback, and return NULL.

@kvark, could you have a look and tell me if it looks right, or if you had something else in mind? I'd be happy to finish the work.

@kvark
Copy link
Member

kvark commented Jan 3, 2022

That's very nice, thank you @sqaxomonophonen ! It will be convenient to provide feedback on a draft PR if you make one.
Edit: general approach looks fine :)

@kvark
Copy link
Member

kvark commented Jan 3, 2022

We don't have push/pop error scopes implemented yet here at wgpu-native level, but @sqaxomonophonen just landed the uncaptured error handler and the device lost handlers in #157.

@rajveermalviya
Copy link
Collaborator

#264 implemented the push/pop error scopes, and now most the internal errors are handle-able by error scopes.
Some that are categorized as fatal errors, still panic: https://github.com/search?q=repo%3Agfx-rs%2Fwgpu-native+handle_error_fatal&type=code
This categorization of fatal errors is copied from wgpu-rs, should we ignore this and avoid panic-ing?

Also we have many .expect() that also panic for checking the args passed via the C api (eg: checking for nulls & enum mappings), should we avoid panic-ing here too? These can be reworked to be handled via error scopes.

cc @cwfitzgerald

@almarklein
Copy link
Collaborator Author

Some that are categorized as fatal errors, still panic: https://github.com/search?q=repo%3Agfx-rs%2Fwgpu-native+handle_error_fatal&type=code

If I interpret that search result, the handle_error_fatal function is never called, is it?

Also we have many .expect() that also panic for checking the args passed via the C api (eg: checking for nulls & enum mappings), should we avoid panic-ing here too? These can be reworked to be handled via error scopes.

Would be nice. Though I find panic-ing on invalid use of the API (as in C-function arguments) much less of a problem.

@rajveermalviya
Copy link
Collaborator

rajveermalviya commented Jun 12, 2023

If I interpret that search result, the handle_error_fatal function is never called, is it?

You have to expand the "Show 19 more matches". Don't know it isn't default in github

@almarklein
Copy link
Collaborator Author

You have to expand the "Show 19 more matches". Don't know it isn't default in github

Ah, I missed that, thanks!

@dominikh
Copy link

What is the current status of this? From what I can tell, there are still code paths that panic even for, AFAICT, valid uses of the API. For example, trying to create an unsupported surface panics with Unsupported Surface. That seems like something that the C side should be able to recover from.

The list of "fatal errors" also seems problematic. For all of those, the C side should be able to fail gracefully, and not have SIGABRT forced on it, the same way that Rust could.

(Arguably it would also be convenient if misuses of the C API wouldn't lead to panics, as this would allow higher level language bindings to wgpu-native to use their native panics/exceptions/errors/... Then again, bindings should probably make sure not to pass invalid data to wgpu-native in the first place.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants