Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On Web, implement Send and Sync for Window and EventLoopProxy #2740

Closed
wants to merge 4 commits into from

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Mar 16, 2023

This is an experiment to let Window implement Send and Sync on Wasm and reuse the same code to implement Sync for EventLoopProxy.
I took some inspiration from MainThreadSafe on macOS.

It's implemented in a way that if you call this from the Window it will do nothing special and just call the appropriate method on the Window, as before. Therefor, except a single Rc<Cell<bool>> to Arc<AtomicBool> change, there is no overhead if Wasm is only used single-threaded.

If used in a multi-threaded environment from a worker, it will use a mpsc::Sender to do it's work on the main thread. Currently this is implemented by just sending a boxed function to execute, but it could be rewritten to just send a message and a message handler would then do the right thing.

When it's necessary to return a value a Condvar and a Mutex is used to await the completion, which would block, but as it's not in the window that should be fine.

Most of the small changes here and there are to disallow calls to web_sys::window(), as these are the main culprits that will fail if not called from a window. This was necessary because it was quite hard to figure out what needs to be changed, so I added a Clippy disallowed-methods, which should hopefully make maintenance of this feature easy. Let me know what you think of this.

Builds on top of #2732 and #2737.

Fixes #2294.

@daxpedda daxpedda force-pushed the web-window-send branch 3 times, most recently from 3eb36d8 to 5b27c40 Compare March 16, 2023 19:30
@kchibisov
Copy link
Member

Just a side note, Winit window is only tested for Send not for Sync so having more could be a bit of confusing.

@daxpedda
Copy link
Member Author

#[test]
fn window_sync() {
// ensures that `winit::Window` implements `Sync`
needs_sync::<winit::window::Window>();
}

Or do you mean more then just this simple test?

@kchibisov
Copy link
Member

Or do you mean more then just this simple test?

Ugh, I guess it's fine then. it's rather strange requirement given that you can't clone it...

I know that you must have Send and I guess I never seen Sync failure on its own.

@daxpedda
Copy link
Member Author

daxpedda commented Mar 16, 2023

it's rather strange requirement given that you can't clone it...

It can be cloned when used in an Arc, but the Arc itself requires Send and Sync to be Send itself (docs). The only way to get around Sync otherwise is to wrap it in a Mutex, which unfortunately breaks Wasm when run on the main thread.

At least that's my use-case, @Liamolucko described some additional requirements in #2294.

@daxpedda
Copy link
Member Author

So I played around with this a bit more, added some comments, prevented leaking unnecessary Weaks and a potential block when cloning.

The only thing left is preventing that one Weak wrapped in ManuallyDrop from leaking, which I have a pretty good idea how to do, but gonna stop wasting my time on this until the other PR's are merged and this one receives a review.

@daxpedda
Copy link
Member Author

daxpedda commented Mar 18, 2023

(CI fails because of duplicated syn dependencies)

@daxpedda daxpedda changed the title On Web, implement Send and Sync for Window On Web, implement Send and Sync for Window and EventLoopProxy Mar 26, 2023
@daxpedda
Copy link
Member Author

daxpedda commented Jun 1, 2023

Closed in favor of #2834.

@daxpedda daxpedda closed this Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants