Set every ContextVK member as it is created in Setup instead of setting all of the members at the end of the method.

If Setup fails, then this will ensure that the ContextVK destructor deletes the objects in the proper order. In particular, objects tied to the device should be deleted before the device, and the Vulkan instance should be deleted last.

Refactor code that doesn't have to do with the change? I'd rather do that as a followup PR if you want.

It's related to this change - in the current version of this PR if Setup exits early it will set device_ in ContextVK but not any other members.

In particular, instance_ is not set by Setup until the end. So if a failure happens during Setup, the instance local to Setup will be destroyed but device_ will not be destroyed until the ContextVK is destructed.

IIUC it's unsafe to call a Vulkan API like vkDestroyDevice if the instance used to create the device has already been destroyed. So I think it would be safest to set all the ContextVK members as they are created and then rely on the ContextVK destructor to delete the context's Vulkan objects in the correct order.

Ah, good observation. That requires that the ivars of the context are in the correct order in the header which at least for the device and instance is the case. It also means the that the order in which the ivars are set in Setup is meaningful. If we space it out across this whole method, it would be easy to get wrong. I'm back to liking the resetter on the stack that I had at first. It makes it easy to verify the order and is the least invasive to what we already have.

I'm just seeing that you are out of office. Let's go over our options when you get back.

Thinking about it, if the order of the ivars is wrong, then the order on the stack would be wrong too. So that doesn't solve the problem. I don't think there is a way you can guarantee this code is safe to all failures. Your proposal is better than what it is now though. I'll make the change.

Starting to implement it, I don't like it since on line X we initialize an instance variable and line X+100 we use it. It isn't clear that that instance variable has been initialized at X+100. By using values on the stack and initializing them when we create them we can't get them out of order. Let's chat when you get back.

There is no way to make this completely foolproof.

But I think the requirement that a constructor must initialize members before it uses them later is well understood. It will be more familiar to readers of the code than the scoped_reset class and the requirement to call release() on the scoped_reset at the right time.

I got rid of the special smart pointer and moved resetting the ivar to a std::unique_ptr. I think you'll find that preferable.

But I think the requirement that a constructor must initialize members before it uses them later is well understood.

It's not that it's not understood, it's impossible to verify when looking at the code without scrolling around because the method is so large.

It will be more familiar to readers of the code than the scoped_reset class and the requirement to call release() on the scoped_reset at the right time.

The nice thing is that they don't have the release, the tests will fail. The order of the release isn't super important as long as it remains in the same spot since the chances of errors in that block of code where all the ivars is getting set is practically zero (possibly zero, i didn't look up std::optional::value or std::string::string's signatures).

Thanks Jason. I have a followup draft PR that will expand the functionality of mock vulkan so that we can assert the behavior when there is an error at setup.

I give up, I can't make Setup fail because in practice we abort if there is any vulkan error. 🤷

I didn't look into it but, I suspect this guy and the FenceWaiterVK may need device holders too.

Use fml::ScopedCleanupClosure here

done