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 for EventLoopProxy #2737

Closed
wants to merge 3 commits into from

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Mar 13, 2023

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

See #2294 for previous discussion on this.

This adjusts EventLoopProxy on Web to be Send, like other platforms.
Because the EventLoop itself is not Send, the way this is solved is by using a channel that receives messages in the background and redirects them to the EventLoop, with the help of wasm_bindgen_futures::spawn_local(), adding a new dependency, even if fairly minor.

Avoiding the wasm-bindgen-futures is kind of impossible because we need async support, but I did build our own async channel with the help of a simple Mutex.

I have some additional ideas in mind:

  • We could avoid the additional dependency by saying that we only support Send when you are using target_feature = "atomics" and otherwise fall back to what we were doing until now. This would add a bunch of cfgs inside the code.

  • With this PR we will always use the channel, even when we are on the main thread. This could be avoided by using something similar like on MacOS with MainThreadSafe. This would also improve the performance overhead this PR introduces by circumventing the channel when not needed.

  • If we don't want to import a big crate like async-channel we could at least import atomic-waker, which has no dependencies and is quite tiny, removing the Mutex.

  • I would like to give the same treatment to Window, which also doesn't implement Send in spite of all other targets having it.

  • I adjusted the web example to send custom user events to try this out, but it's written in a way to also be able to compile on other platforms, which adds a lot of cfgs everywhere, not sure if I should add it here:

    Adjusted Example
    --- a/examples/web.rs
    +++ b/examples/web.rs
    @@ -1,13 +1,53 @@
    #![allow(clippy::single_match)]
    
    +use std::time::Duration;
    +
    +#[cfg(not(wasm_platform))]
    +use std::thread;
    +#[cfg(wasm_platform)]
    +use wasm_bindgen::{closure::Closure, JsCast};
    use winit::{
        event::{Event, WindowEvent},
    -    event_loop::EventLoop,
    +    event_loop::EventLoopBuilder,
        window::WindowBuilder,
    };
    
    +const INTERVAL: Duration = Duration::from_millis(2500);
    +
    +#[derive(Clone, Copy, Debug)]
    +pub struct Counter(u64);
    +
    pub fn main() {
    -    let event_loop = EventLoop::new();
    +    #[cfg(not(wasm_platform))]
    +    simple_logger::SimpleLogger::new().init().unwrap();
    +
    +    let event_loop = EventLoopBuilder::with_user_event().build();
    +    let proxy = event_loop.create_proxy();
    +    let mut counter = Counter(0);
    +
    +    let mut sender = move || {
    +        let result = proxy.send_event(counter);
    +        counter.0 += 1;
    +        result
    +    };
    +
    +    #[cfg(not(wasm_platform))]
    +    thread::spawn(move || loop {
    +        thread::sleep(INTERVAL);
    +        if let Err(_) = sender() {
    +            break;
    +        }
    +    });
    +    #[cfg(wasm_platform)]
    +    let closure = { Closure::<dyn FnMut()>::new(move || sender().unwrap()) };
    +    #[cfg(wasm_platform)]
    +    let handle = web_sys::window()
    +        .unwrap()
    +        .set_interval_with_callback_and_timeout_and_arguments_0(
    +            closure.as_ref().unchecked_ref(),
    +            INTERVAL.as_millis().try_into().unwrap(),
    +        )
    +        .unwrap();
    
        let window = WindowBuilder::new()
            .with_title("A fantastic window!")
    @@ -20,14 +60,25 @@ pub fn main() {
        event_loop.run(move |event, _, control_flow| {
            control_flow.set_wait();
    
    +        log::debug!("{:?}", event);
    +
            #[cfg(wasm_platform)]
    -        wasm::log_event(&log_list, &event);
    +        {
    +            let _closure = &closure;
    +            wasm::log_event(&log_list, &event);
    +        }
    
            match event {
                Event::WindowEvent {
                    event: WindowEvent::CloseRequested,
                    window_id,
    -            } if window_id == window.id() => control_flow.set_exit(),
    +            } if window_id == window.id() => {
    +                #[cfg(wasm_platform)]
    +                web_sys::window()
    +                    .unwrap()
    +                    .clear_interval_with_handle(handle);
    +                control_flow.set_exit();
    +            }
                Event::MainEventsCleared => {
                    window.request_redraw();
                }
    @@ -38,6 +89,7 @@ pub fn main() {
    
    #[cfg(wasm_platform)]
    mod wasm {
    +    use super::Counter;
        use wasm_bindgen::prelude::*;
        use winit::{event::Event, window::Window};
    
    @@ -71,20 +123,22 @@ mod wasm {
            log_list
        }
    
    -    pub fn log_event(log_list: &web_sys::Element, event: &Event<()>) {
    -        log::debug!("{:?}", event);
    -
    +    pub fn log_event(log_list: &web_sys::Element, event: &Event<Counter>) {
            // Getting access to browser logs requires a lot of setup on mobile devices.
            // So we implement this basic logging system into the page to give developers an easy alternative.
            // As a bonus its also kind of handy on desktop.
    -        if let Event::WindowEvent { event, .. } = &event {
    -            let window = web_sys::window().unwrap();
    -            let document = window.document().unwrap();
    -            let log = document.create_element("li").unwrap();
    -            log.set_text_content(Some(&format!("{event:?}")));
    -            log_list
    -                .insert_before(&log, log_list.first_child().as_ref())
    -                .unwrap();
    -        }
    +        let event = match &event {
    +            Event::WindowEvent { event, .. } => format!("{event:?}"),
    +            Event::UserEvent(event) => format!("{event:?}"),
    +            _ => return,
    +        };
    +
    +        let window = web_sys::window().unwrap();
    +        let document = window.document().unwrap();
    +        let log = document.create_element("li").unwrap();
    +        log.set_text_content(Some(&event));
    +        log_list
    +            .insert_before(&log, log_list.first_child().as_ref())
    +            .unwrap();
        }
    }

Partially replaces #2294.
Fixes #2592.

@daxpedda daxpedda changed the title On Web, EventLoopProxy now implements Send On Web, implement Send for EventLoopProxy Mar 15, 2023
match self.receiver.try_recv() {
Ok(event) => Poll::Ready(Ok(event)),
Err(TryRecvError::Empty) => {
*self.waker.lock().unwrap() = Some(cx.waker().clone());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use will_wake here in order to avoid unnecessarily cloning a waker twice.

Copy link
Member Author

@daxpedda daxpedda Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about this! I also noticed that neither futures or atomic-waker use this this optimization in their AtomicWaker implementation. I opened an issue here: smol-rs/atomic-waker#11.

In our case this won't be possible because to avoid a racing condition we would have to hold the lock, which can lead to blocking in a multi-threaded environment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you do:

{
    let mut waker = self.waker.lock().unwrap();
    if waker.map_or(true, |waker| !waker.will_wake(cx.waker())) {
        *waker = Some(cx.waker().clone());
    }
}

in order to drop the lock once the temporary scope ends?

Copy link
Member Author

@daxpedda daxpedda Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't concerned with how to drop the lock, but the fact that we hold it in the first place. The assumption in play here is that if you only assign a value without any checks in-between, optimizations can prevent this to block the main thread because it won't yield in-between a simple assignment. Not really a provable assumption, nor one I'm certain about, but it's what we have right now without using AtomicWaker.

If the the worker yields between holding the lock and comparing and cloning the Waker, we are gonna have a problem.

src/platform_impl/web/event_loop/runner.rs Outdated Show resolved Hide resolved

pub struct AsyncSender<T: 'static> {
sender: Sender<T>,
waker: Arc<Mutex<Option<Waker>>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Mutex available on WASM? I always forget what synchronization primitives panic on block for non-atomic WASM or not. Although, it looks like that, in single threaded cases, it shouldn't block. Is this right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Mutex available on WASM?

Yes.

Although, it looks like that, in single threaded cases, it shouldn't block. Is this right?

For single threaded cases it shouldn't block indeed, but it can for multi-threaded cases. Considering we are just storing a single value and not holding a lock in practice it's not a problem.

I would still like to propose that we don't use a Mutex here and use AtomicWaker instead. The crate has no dependencies and is really small.

If the dependency is still a concern, I would like to point to my suggestion above: hiding the Send support behind the +atomics target feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would support using atomic-waker, but I'd ask the rest of the maintainers first.

@daxpedda
Copy link
Member Author

CI fails on iOS because of warning: spurious network error (2 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io).

@notgull
Copy link
Member

notgull commented Mar 15, 2023

CI fails on iOS because of warning: spurious network error (2 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io).

Yeah, the iOS CI check does that sometimes. It's not your fault

@daxpedda daxpedda mentioned this pull request Mar 26, 2023
@daxpedda
Copy link
Member Author

Closing in favor of #2740.

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.

Not possible to send events to winit from a Web Worker
2 participants