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

Filament panics unrecoverably in background threads for potentially recoverable issues #8197

Open
emezeske opened this issue Oct 11, 2024 · 0 comments
Assignees
Labels
enhancement New feature or request low priority Low priority issue

Comments

@emezeske
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Yes. See #8185 for the related problem.

I read through the big comment in Panic.h explaining what it's used for, and it's well-conceived. But it seems that Filament does not universally follow the principles laid out in that comment.

In relation to #8185, Filament is panicing via ASSERT_POSTCONDITION when vkCreateSwapchainKHR returns any error. This is not necessarily an arithmetic error, programming error, contract error, etc. There are many possible causes, including things like device resources being exhausted. That does not seem like a valid reason to unrecoverably panic/terminate.

NB that even defining a custom Panic handler does not make these things recoverable, unless they happen on the user's main render thread. If ASSERT_POSTCONDITION is it for a soft error like this inside one of Filament's internal threads, there's nothing the user can do to recover -- either an exception will be thrown that the user cannot catch (because they don't own the thread), or abort() will be called.

In my toy example of vkCreateSwapchainKHR failing in an internal Filament thread, the ideal behavior would be that the user code could handle this error. For example, the user code could destroy the Vulkan renderer and retry, possibly with a different backend. (It's quite likely that destruction of the Vk device would free the leaked device swapchain memory). Or at the very least, an error message could be presented to the user instead of an app crash.

Describe the solution you'd like
Catch Panic exceptions in Filament background threads and re-throw those exceptions inside the user's thread so that the calling code can handle them appropriately. For example, the internal exceptions could be re-thrown in endFrame() or beginFrame() or similar. This doesn't seem super-difficult and would provide a way for user code to respond to issues.

Describe alternatives you've considered
Audit the use of Panic and eliminate its use for soft errors, especially for things like graphics API calls that can fail for environmental reasons that are not necessarily programmer error, and especially for any code that can run in the Filament background threads where Panic exceptions cannot be caught by the user. Panic/CHECK fail has its place, but it's for the kinds of reasons explained in Panic.h: true programming errors, especially those that are likely to leave the entire process in an unknown state due to undefined behavior, etc.

OS and backend
Not OS-specific or backend-specific.

@pixelflinger pixelflinger added the enhancement New feature or request label Nov 5, 2024
@pixelflinger pixelflinger self-assigned this Nov 5, 2024
@pixelflinger pixelflinger added the low priority Low priority issue label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request low priority Low priority issue
Projects
None yet
Development

No branches or pull requests

2 participants