Make default WindowBuilder Send and Sync
#3136
Conversation
kchibisov
requested review from
jackpot51,
msiglreith,
daxpedda and
madsmtm
as code owners
There was a problem hiding this comment.
I disagree that this is the right way to solve this issue, window handles are thread safe on macOS/iOS, since we guarantee to never use them outside the main thread.
Rather, we should wrap the window handle in something that implements Send + Sync (or maybe we should figure it out in rust-windowing/raw-window-handle#85 instead).
|
@madsmtm the issue is not only a window handle as it was said. You can see the CI logs and discover, that all the Web can't be Send, what should we do about it? As well as monitors on macOS. |
|
Like, surely, we could keep the |
|
I believe the solution is the same on the web: Since the window handle can only be created and accessed from the main thread, then it doesn't matter if the handle was sent between threads (or service workers) in between those accesses. @daxpedda?
I think it's the same thing here, monitors can be |
There was a problem hiding this comment.
I'm against introducing this generic, we already have solutions in place for this kind of issue in MacOS and Web, we just need to apply them here as well as @madsmtm already suggested.
For Web, we just need to wrap the innards into MainThreadSafe, that's it. I'm happy to figure it out once NotSendSyncWindowAttributes is removed.
kchibisov
force-pushed
the
default-builder-send-sync
branch
2 times, most recently
from
38f7b29
to
fa3aabe
Compare
There was a problem hiding this comment.
I'm only not sure what to do with the video mode stuff, Like it's not send/sync in general, but given that macOS will allow creating it only on the main thread, and then winit will use it on the main thread, I think just marking Fullscreen in builder should be fine.
There probably better solutions, but searching for macOS docs on what is MT-safe and what is not is really hard.
src/lib.rs
Outdated
| /// mean thread if the platform demands it. Thus marking such objects as `Send + Sync` is safe. | ||
| #[doc(hidden)] | ||
| #[derive(Clone, Debug)] | ||
| pub struct SendSyncWrapper<T>(pub T); |
There was a problem hiding this comment.
Not sure whether this object should be public at all, the only reason it's public is because we allow access to WindowAttributes and also building them with struct builder.
We could make it crate private and just provide a get/set for parent` on WindowAttributes?
kchibisov
force-pushed
the
default-builder-send-sync
branch
from
fa3aabe
to
3f605e7
Compare
Window builder is always accessed by winit on the thread event loop is on, thus it's safe to mark the data it gets as `Send + Sync`. Each unsafe object is marked individually as `Send + Sync` instead of just implementing `Send` and `Sync` for the whole builder.
kchibisov
force-pushed
the
default-builder-send-sync
branch
from
3f605e7
to
8e6b336
Compare
|
This is broken on Web and shouldn't be EDIT: False alarm, I forgot |
|
Exactly, we discussed that, it's always used on the thread event loop is on. |
The default window builder is neither
SendnorSyncdue to use ofRawWindowHandleinternally. While we could ensure to make the handleSend + Syncwithin our use, we can't guarantee that the same will hold true in the future.Thus add a generic to split the builder into
MT-safeand notMT-safeversions.This could also help with the lifetimes, since default could be bound by static lifetime.
CHANGELOG.mdif knowledge of this change could be valuable to users