Skip to content

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

Description

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

Labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions