gfx-rs / wgpu-native

Native WebGPU implementation based on wgpu-core
Apache License 2.0
879 stars 105 forks source link

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

Open almarklein opened 3 years ago

almarklein commented 3 years ago

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 commented 3 years ago

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.

sqaxomonophonen commented 2 years ago

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

https://github.com/sqaxomonophonen/wgpu-native/commit/5ebd50e828b726b58394ee30f0b6a352f0c0c6a0

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Ah, I missed that, thanks!

dominikh commented 6 months ago

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.)