search

linebender / piet

An abstraction for 2D graphics.
Apache License 2.0
1.24k stars 93 forks source link

piet-direct2d `Send` impls are unsound? #488

Closed cmyr closed 2 years ago

cmyr commented 2 years ago

the ComPtr type is explicitly marked !Send, so our send impls are sketchy. This is currently a hard failure on clippy.

Additionally, the winrt crate is deprecated. In theory we should move to the windows crate; I'm not sure how involved that would be as a rewrite.

The quickest solution would be to remove our Send impls, which would be a breaking change. I can't really think of a better short-term solution, though. :(

jneem commented 2 years ago

I don't know anything about ComPtr (is it any worse than, say *mut?), but this clippy lint is known to have plenty of false positives, and it's going to get removed.

raphlinus commented 2 years ago

I am also in favor of suppressing the clippy lint. The COM safety story is complex, but I think we should model it so that send is considered safe, clone of mutable objects is not safe, and we try to treat a lot of resources as immutable even if there are mutation methods. This is particularly tricky for text layouts, where I wouldn't be surprised if our best bet was doing our own Cow (so two layers of reference counting, the inner COM one is always at 1).

To some extent, allowing multithreaded use of Piet is unfinished work. We've always allowed it to some extent but with big limitations (most access to RenderContext is &mut self, we need a separate context which has &self resource creation methods). The endgame in my opinion definitely facilitates multithreading, and that means most objects being Send.

raphlinus commented 2 years ago

I think the lint is gone in Rust 1.58.1?

cmyr commented 2 years ago

Okay I'm happy to just put my blinders back on and forget this ever happened.

derekdreery commented 2 years ago

I spent some time looking at ComPtr, and it's it just true that it's threadsafe. It seems to have the same semantics as std::sync::Arc.

It uses interlocked functions to update the ref count, which seems to be the same as Ordering::SeqCst (it explicitaly mentions Acquire and Release being weaker versions of the sync).

Is this correct?