Add reference counted handles#207
Conversation
|
Probably opened in a hurry to compare, but do note (in the description) that this seems to be an alternative approach to #206 (is sort-of a duplicate)? |
|
This is meant to be an implementation of "outer reference counting", as opposed to #206, which is inner reference counting. |
144f938 to
ff925be
Compare
|
I feel like we've overcome the hard part in #188, deciding on the semantics (Reference-Counted Control), now it's "just" the syntax. Something that I feel we need to at least consider here is: What is the value of I imagine the answer to be "it allows the user to implement the reference-counting in a different manner" (including in C probably), i.e. we're not tied to The downsides as I see from that is mainly that you cannot express The natural question then becomes: Are the benefits worth it compared to the downsides? |
I believe the downsides are nullified/minimized in that we provide very convenient conversions in RWH. If there are other common cases out there we should consider adding conversion methods in RWH to cover them. So in my eyes the current implementation retains maximum flexibility while at the same time keeping real-life scenarios as convenient as possible. |
|
If C interop is the main motivator, then I think that can be solved with a custom I think the main difference is if we actually expose the owned handle as the return type anywhere in our API, which I think I'm somewhat proposing in #200 - then whatever handle we use there kinda has to be the maximally flexible one (or a generic?). (To be clear, I'm in favour of a separate |
1cd8bd3 to
4b36004
Compare
The VTable gives you more flexibility over how the underlying pointer acts. For example, see the |
| assert_impl_all!(WindowHandle: Send, Sync); | ||
| assert_impl_all!(DisplayHandle: Send, Sync); | ||
| assert_impl_all!(WindowVtable: Send, Sync); | ||
| assert_impl_all!(DisplayVtable: Send, Sync); |
There was a problem hiding this comment.
Check for UnwindSafe + RefUnwindSafe + Unpin as well?
Or them not having it, the important part is that its cross-platform consistent.
There was a problem hiding this comment.
I don't think we've decided on unwind semantics for any of the handles so far. We should probably take this in another MR.
There was a problem hiding this comment.
Even if we don't decide on it, we want it to be consistent on all platforms in the meantime.
We don't want to end up in a scenario where a users code suddenly stops compiling when targeting a specific platform. This has even happened with Std types before.
There was a problem hiding this comment.
Rebasing seems to have accidentally removed these checks for the new types btw?
And I agree that we should check these in a test, regardless of whether we want the impls or not.
4b36004 to
55465e2
Compare
| /// An owned window handle. | ||
| pub struct WindowHandle { | ||
| /// Underlying data representative of the window handle. | ||
| ptr: NonNull<()>, |
There was a problem hiding this comment.
What's the reasoning for NonNull<()> here? RawWaker seems to use *const ().
I guess if people aren't using it, they can always pass NonNull::dangling(), so it might be nice for niche optimizations?
There was a problem hiding this comment.
No real point aside from have an extra point of nicheing. The handle type is already niched because of the &'static VTable. Frankly I could go either way; I just don't see the point of null() when dangling() exists.
There was a problem hiding this comment.
I guess I just don't really like NonNull<()>, just like I don't like NonZero<...>, it's always a bit of a bother to use because you have to import it and call constructors and getters (whereas *const has coercions).
Though I guess it's not that big of a deal, users are hopefully going to mostly use the From<Arc> helper.
There was a problem hiding this comment.
My current opinion is that removing the possibility of the handle to be null potentially saves more footguns than allowing it.
3dadee6 to
04b2cce
Compare
049fa73 to
9b41212
Compare
| /// On some platforms, it's possible to separate the underlying producer | ||
| #[doc = concat!("of a ", $type, " from the actual certain pointer. As an example,")] | ||
| /// on macOS you could choose to implement the vtable by [`retain`](https://docs.rs/objc2/latest/objc2/trait.Message.html#method.retain)ing | ||
| /// or [`release`](https://docs.rs/objc2/latest/objc2/rc/struct.Retained.html#method.drop)ing | ||
| /// the underlying Objective-C object. On WASM platforms, you could implement | ||
| /// this value [`JsValue`](https://docs.rs/wasm-bindgen/latest/wasm_bindgen/struct.JsValue.html)'s | ||
| /// natural refcounting. This strategy can be used as an optimization and | ||
| #[doc = concat!("is not unsound. However, it should be noted that dropping ", $type, "s")] | ||
| /// when such handles are held varies between platforms. |
There was a problem hiding this comment.
This doc doesn't really makes sense on display handles.
There was a problem hiding this comment.
It could in the future, if we shove macOS/web bits into the display handle.
There was a problem hiding this comment.
As in all the other PRs I also believe we should remove these docs.
If we want to future-proof against making them suddenly Send + Sync in the future, we can just say something like "We reserve the right to change data-less handles to Send + Sync without a breaking change.".
9b41212 to
30a6cdb
Compare
8b8fdd7 to
983cf43
Compare
883bf9b to
e298090
Compare
| /// On some platforms, it's possible to separate the underlying producer | ||
| #[doc = concat!("of a ", $type, " from the actual certain pointer. As an example,")] | ||
| /// on macOS you could choose to implement the vtable by [`retain`](https://docs.rs/objc2/latest/objc2/trait.Message.html#method.retain)ing | ||
| /// or [`release`](https://docs.rs/objc2/latest/objc2/rc/struct.Retained.html#method.drop)ing | ||
| /// the underlying Objective-C object. On WASM platforms, you could implement | ||
| /// this value [`JsValue`](https://docs.rs/wasm-bindgen/latest/wasm_bindgen/struct.JsValue.html)'s | ||
| /// natural refcounting. This strategy can be used as an optimization and | ||
| #[doc = concat!("is not unsound. However, it should be noted that dropping ", $type, "s")] | ||
| /// when such handles are held varies between platforms. |
There was a problem hiding this comment.
As in all the other PRs I also believe we should remove these docs.
If we want to future-proof against making them suddenly Send + Sync in the future, we can just say something like "We reserve the right to change data-less handles to Send + Sync without a breaking change.".
| /// The design of this type makes it possible to drop handles on other | ||
| /// threads. For some platforms (Win32, macOS, etc) this can be problematic, | ||
| /// since handles have ["thread affinity"](https://devblogs.microsoft.com/oldnewthing/20051010-09/?p=33843) | ||
| /// that ties them to their origin thread. Therefore, for these types, it | ||
| /// is on the onus of the producer to send the window handle back to its | ||
| /// origin thread to be dropped. |
There was a problem hiding this comment.
I know this isn't true on Windows. You need to drop the window in the correct thread, not the handle.
What about macOS? Does the actual handle really need to be dropped in the main thread @madsmtm?
There was a problem hiding this comment.
You must dealloc NSWindow on the same thread that you created it on (and there's a bunch of restrictions on it if that thread isn't the main thread), and UIWindow must be created and deallocated on the main thread.
But the reference-counting itself (retain/release) is thread-safe. So as long as you don't call call release when the reference-count is 1, and while not being on the main/creator thread, everything is fine.
Alternatively, you can store weak references, but they can only be upgraded as long as you know you're not going to be the last one to call release.
(And you can get around the whole issue by dispatch-ing the release to happen on the main thread, you could probably do it async too so that you'll instead leak if the main thread's event loop isn't running).
There was a problem hiding this comment.
So I think we keep it.
cb1f4e2 to
1a6a5cd
Compare
This adds "WindowHandle" and "DisplayHandle" types as discussed in #188. These act similar to `Waker`'s in stdlib, as they are a wrapper around a trait object. Signed-off-by: John Nunley <dev@notgull.net>
1a6a5cd to
6cd5b63
Compare
Fixes #188