RFC: Switch to a futures-compatible API

[tomaka]

Winit's and glutin's current API look like this:

impl Window {
    pub fn wait_events(&self) -> impl Iterator<Item = Event> { ... }
    pub fn poll_events(&self) -> impl Iterator<Item = Event> { ... }
}

I suggest that instead you'd have to register your window to what's called an events loop. Then, you'd call a method on the events loop to run it. Running the events loop would process events received by the operating system, and call a callback previously registered on the window.

This is essentially the same approach as the future-rs library, except that we're not using future-rs directly because it's not totally stable. I'd like to eventually publish winit 1.0 (in, like, the next nine months or so), and it's unlikely that future-rs publishes its 1.0 before winit.

Example usage with the new API (just to give an overview, the details are not important at the moment):

let events_loop = EventsLoop::new();
let window = WindowBuilder::new().with_title("hello").build(&events_loop).unwrap();
window.set_callback(|event| match event { ... });

loop {
    events_loop.run_once();     // calls the closure passed to set_callback earlier
}

I've always been a bit wary about callbacks because they are often too "magical", like you don't know which thread calls them and at what moment. But the fact that it's the run() method of the events loop that calls the callbacks means that it's a good design in my opinion.

This approach would have some benefits over the current system:

• In the implementation we can decouple the Window object in Rust from the actual window. The actual window could be stored in the EventsLoop for example.
• Right now on Windows winit spawns a background thread for events handling ; this could be removed.
• We can force the EventsLoop to run on the main thread for OSX while still allowing the Window objects to be shared between threads. This has been a huge safety hole in glutin since the beginning.
• The set_resize_callback function of MacOS would be gone.
• The WindowProxy would be gone in favor of an "EventsLoopProxy".
• We can later register other kinds of callbacks to the events loop, for example a callback when a monitoring is plugged in or removed that is not tied to particular window.
• This design is also more appropriate for emscripten in glutin, as emscripten/javascript are based on callbacks and not on preemptive multitasking.

[tomaka]

I guess I'll ping some people to get some opinions: @ozkriff @mitchmindtree @kvark @fkaa @vberger @glennw @larsbergstrom

[Osspial]

How would this allow winit to not spawn a background thread on Windows? Rescaling a window causes the callback thread to be blocked until the end of the rescale, a behavior which is currently hidden by the background thread, and as far as I know there isn't any way to stop that behavior. Moving it onto the main thread would move the blocking there.

[mitchmindtree]

There are two main trade-offs that come to mind with this approach:

• Control Flow - a user may want to stop checking for events after a certain event type is received, or return early using the try! macro following some error. It can be very tedious to handle this when the user is restricted to the scope of a closure. How do you anticipate handling control flow with this design? I'm not sure I'm familiar enough with futures-rs to know how they deal with it.
• Ownership issues - I'm guessing that most of these callbacks will be constrained by FnMut? I can imagine this making it difficult to "share" data between each of the callbacks. It seems like we'd either have to require users wrap their "shared" data in Rc<RefCell<>> or Arc<Mutex<>>, or that they propagate the events yielded by each callback over some channel to a loop that looks like what glutin currently looks like. I put "shared" in quotations as the closures are only actually ever called from the same thread, right?

In the implementation we can decouple the Window object in Rust from the actual window. The actual window could be stored in the EventsLoop for example.

I'm curious why this is a benefit? Is it because you're unsure how to both make the window handle available to the user while also using it to run the events loop? Another way (using glutin's current style) might be to pass the window to the method that yields the next event, something like:

loop {
    let poll = events_loop.poll();
    while let Some(event) = poll.next(&mut window) {
        // Still have access to the window handle
    }
}

I'm not familiar enough with the issues related to the other benefits you mentioned to be able to comment on whether they are worth the trade-offs I mentioned above.

[tomaka]

@Osspial Are you sure about that? On Win32 you're supposed to poll and dispatch the messages manually with PeekMessage and DispatchMessage, so I'm not sure how it would block.

@mitchmindtree Don't forget that the closure can borrow its environment.
You could write for example this:

window.set_callback(|ev| {
    match ev {
        Event::Closed => events_loop.stop(),
        _ => ()
    }
});

events_loop.run_until_stopped();

Or, this:

let mut events = RefCell::new(Vec::new());
window.set_callback(|ev| events.borrow_mut().push(ev));

loop {
    events_loop.run_once();
    for event in events.borrow_mut().drain(..) {
        ...
    }
}

It's true that if you want to isolate the window in a struct, then you will need a Rc<RefCell<>> or an Arc<Mutex<>> like this:

pub struct MyApp {
    events: Arc<Mutex<Vec<Events>>>,
    window: Window,
}

impl MyApp {
    pub fn new(events_loop: &EventsLoop) -> MyApp {
        let events = Arc::new(Mutex::new(Vec::new()));
        let events2 = events.clone();
        let window = Window::new(events_loop);
        window.set_callback(move |ev| events2.lock().unwrap().push(ev));
        MyApp { events: events, window: window }
    }
}

However I think this use case doesn't happen often.

I'm curious why this is a benefit?

The benefit of decoupling the window from the events loop is that on MacOS we can run the events loop on the main thread and the window in another thread, and that with X11 we can do everything in a single-threaded fashion by creating one X connection per EventLoop, which will likely lead to less bugs in xlib.

[Osspial]

@tomaka Yeah. I was able to get winit working without multithreading on Windows with the current API, and everything worked fine except for rescales, which blocked execution. Apparently, Windows enters an internal loop during rescales that does end up calling the user-defined window procedure, but doesn't return from DispatchMessage, effectively blocking program execution until the resize ends, exiting the loop.

[tomaka]

One interesting problem is about leak safety.
In the example above, if we mem::forget the Window and run the events loops, we can access the objects referenced by the callback without having any actual borrow on them. This is undefined behavior.

Future-rs's tasks system is very complex and avoids this problem by running the callback only if we call a method on either the future itself or the task that holds the future.

[mitchmindtree]

Looking at this issue again and looking over the benefits you mention, I think they're easily worth the issues I mentioned, and the workarounds you shared look really reasonable anyways. 👍 from me.

[tschneidereit]

@tomaka would it make sense to actually make this use futures-rs at this point. According to @aturon Futures should be quite stable at this point. (Also note @alexcrichton's followup on tokio-core stability.

[aturon]

@tschneidereit I'd be happy to advise more specifically if you decide to go this direction!

[tschneidereit]

@aturon thanks! I guess the main question is whether it's prudent to plan on releasing a 1.0 of winit based on futures-rs within the next nine months or so:

This is essentially the same approach as the future-rs library, except that we're not using future-rs directly because it's not totally stable. I'd like to eventually publish winit 1.0 (in, like, the next nine months or so), and it's unlikely that future-rs publishes its 1.0 before winit.

@tomaka, do I understand correctly that with the right stability guarantees you'd go with futures-rs?

[tschneidereit]

@pcwalton, do you think a Futures-/Tokio-based event loop for winit would work well for Servo?

[aturon]

I guess the main question is whether it's prudent to plan on releasing a 1.0 of winit based on futures-rs within the next nine months or so

I think that's reasonable, yes, assuming that today's futures-rs seems to cover your needs. We definitely won't be removing any functionality, and at this point the core future and stream abstractions have gotten quite a bit of vetting. While I do expect a 0.2 release in the coming months, we're already using features like deprecation warnings to make that a smooth transition, and I don't expect core abstractions to be changes much, if at all -- with the exception of the just-added Sink trait, which will likely need a bit more design iteration.

[glennw]

At first reaction, this seems like it would be fine for Servo.

The main parts for us are:

• The EventLoopProxy support, which is vital, but sounds like it's just a simple API change.
• I don't work on mac often, but I recall running in to lots of issues with the synchronous resize callback (I don't recall exactly what they were, unfortunately). So long as we retain the functionality to be able to do GL rendering while a resize is ongoing, that should be fine.

[asajeffrey]

This is on my to-do list, probably to look at starting Q1 2017.

[tomaka]

I'm still a bit hesitant about depending on future, though. I don't want to jump into semver hell by exposing another library's internals. When some person's code will inevitably break after they run cargo update, I will get pinged on IRC to tackle the problem.

I'm tempted to either wait for the private/shared dependencies system that was advertised in a talk recently, or for futures to be merged in the stdlib.

[tomaka]

I've been thinking about it, and for me the future library has too many drawbacks:

• Complicated to use.
• You totally lose sense of what's happening. For example good luck knowing where you will land if you happen to panic in a future handler. Since OpenGL operations and some windowing operations are local to a thread, it's problematic. For example trying to call an OpenGL function from a callback could be very dangerous. I think futures/tasks and regular threads are not really compatible.
• The fact that futures always return a Result even though errors can't happen. The poll_events and wait_events functions don't return a Result.
• The tasks system has the consequence that you need to spawn one thread for each event loop that you have in your program. Who wants to have their main thread dedicated entirely to handling windowing messages? With my proposal in the opening post you could use a loop to run each event loop once.

If you except the leakpocalypse-related problem described above, I don't see any good reason to use futures instead of a custom API. On the other hand, it's very easy to wrap the API described in the opening post around futures if one wants to use futures.

[tomaka]

Maybe an API like this is better:

let events_loop = EventsLoop::new();
let window1 = WindowBuilder::new().build(&events_loop).unwrap();
let window2 = WindowBuilder::new().build(&events_loop).unwrap();

loop {
    events_loop.run_once(|event| {
        match event {
            Event::Resized { window_id, ... } => {
                if window_id == window1.id() { println!("win 1 resized"); }
                else { println!("win 2 resized") }
            },
            Event::MonitorConnected { ... } => {},
            _ => {}
        }
    });
}

The events loop calls a local callback and each event contains an identifier of the window that produced the event (amongst the windows that were registered on the events loop).

I think it would still provide all the advantages described above. We still need to use a callback and not return events by value because of the MacOS resize callback problem, and for emscripten.

Someone would really wants to use futures could easily implement Future/Stream over this API by taking control of the events loop's callback.

[norru]

Perhaps this is of interest: https://github.com/gamazeps/RobotS

[norru]

rust-lang/futures-rs#50

[mitchmindtree]

@tomaka that API looks fine to me!

Would it be worth making a new window-specific event enum to differentiate between general events and events that apply to a specific window? It might save adding a WindowId field to half of the events. E.g. the existing events might be split up something like this:

pub enum Event {
    MonitorConnected,
    MonitorDisconnected,
    KeyboardInput,
    Awakened,
    Suspended,
    Window { id: WindowId, event: WindowEvent },
}

pub enum WindowEvent {
    Focused,
    Resized,
    Moved,
    Closed,
    DroppedFile,
    MouseMoved,
    MouseEntered,
    MouseLeft,
    Touch,
    TouchpadPressure,
}

So matching on events might look something like this

match event {
    winit::Event::MonitorConnected { ... } => (),
    winit::Event::KeyboardInput { ... } => (),
    winit::Event::Window { id, event } => match event {
        winit::WindowEvent::Resized { ... } if id == window1 => println!("win 1 resized"),
        winit::WindowEvent::Closed if id == window2 => println!("win 2 closed"),
        _ => ()
    },
    _ => (),
}