"Safety issue" with (some?) handle types implementing Sync + Send

[schwa423]

I noticed that the handle types generated by the define_handle! macro add Sync + Send to the type in a way that is technically unsafe. For example, it is illegal in Vulkan to concurrently add commands to a CommandBuffer from multiple threads. Yet, since a CommandBuffer handle is both Clone and Send, it is possible for applications to perform this illegal action.

I'm not sure what the right action here is. I know that ash doesn't have the goal of enforcing complete safety via the type system (there's Vulkano for that). I haven't thought deeply about the DX implications of trying to remove Sync and/or Send from (some) handle types, but it doesn't seem like it could happen without significant repercussions.

Perhaps it would be desirable/sufficient to document this "gotcha" somewhere like macros.rs?

[Ralith]

Vulkan objects generally have nontrivial concurrency properties that, as with all of Vulkan's other myriad soundness rules, are your responsibility to enforce. It's not obvious to me that this even merits a special callout, since it goes without saying that you must refer to the spec to determine correct usage of any nontrivial Vulkan interface.

[filnet]

And there are exception to the general concurrency rules.

For example, vkWaitForPresentKHR :

As an exception to the normal rules for objects which are externally synchronized, the swapchain passed to vkWaitForPresentKHR may be simultaneously used by other threads in calls to functions other than vkDestroySwapchainKHR. Access to the swapchain data associated with this extension must be atomic within the implementation.

So enforcing such rules in ash would break things.

This is where the Vulkan Validation Layer comes in. It does all the soundness checks so you don't have to.
Afaict, the checks cover the "externally synchronized" base rule.
I did find an issue concerning the vkWaitForPresentKHR exception: KhronosGroup/Vulkan-ValidationLayers#3615

[Ralith]

It does all the soundness checks so you don't have to.

The validation layers are like a sanitizer; they find some issues, but will never be able to check literally everything.

[filnet]

It does all the soundness checks so you don't have to.

The validation layers are like a sanitizer; they find some issues, but will never be able to check literally everything.

Agreed. That said VVL does a great good job and I can't imagine not using it.

[schwa423]

Do validation layers do a good job of catching threading errors? Just curious. Anyway...

The issue is that most Rust libraries work with the type system so that if it compiles you can be fairly confident that there are no threading or memory errors. ash doesn't do this, and for good reasons, but it doesn't mention this anywhere in the documentation that I can see.

Now that I think about it, my original suggestion to document in macros.rs is way too hidden. It would be better to document on the main ash page that it isn't aiming to support Rust's famous "fearless concurrency". I suppose maybe this is what is meant in the Overview section where it says "No validation, everything is unsafe", but this isn't crystal-clear, and there is no elaboration later on.

Probably most Vulkan programmers will "intuitively" sense that ash doesn't insulate them from such inherent Vulkan requirements, and will program it much the same that they would in C++ (I intuitively understood, FWIW). Nevertheless, it doesn't seem like a bad idea to be extra-clear that ash doesn't aspire to meet standard community expectations around thread/memory safety. But I won't be very unhappy if you just decide to close this issue.

[MarijnS95]

The consensus is that, because Vulkan defines concurrency on a per-command rather than per-object basis, removing Send+Sync from handles will make it impossible to use in multithreaded contexts. Ash is a low-level wrapper without validation, and that is communicated by all commands being unsafe. We even added Send+Sync on all Vulkan structures for the same reason: #869

To answer your request for extra documentation, this is already mentioned in the README:

No validation, everything is unsafe

Which includes concurrency, even if not mentioned explicitly.