From a1da85a24b2bb2493ce95a949d6ada5b834f7d0a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 10 Oct 2018 12:41:26 -0700 Subject: [PATCH] Defer future execution to the next event loop tick Previously whenever a future readiness notification came in we would immediately start polling a future. This ends up having two downsides, however: * First, the stack depth may run a risk of getting blown. There's no recursion limit to defer execution to later, which means that if futures are always ready we'll keep making the stack deeper. * Second, and more worrisome in the near term, apparently future adapaters in the `futures` crate (namely the unsync oneshot channel) doesn't actually work if you immediately poll on readiness. This may or may not be a bug in the `futures` crate but it's good to fix it here anyway. As a result whenever a future is ready to get polled again we defer its polling to the next turn of the event loop. This should ensure that the current call stack is always drained and we're effectively enqueueing the future to be polled in the near future. --- crates/futures/src/lib.rs | 33 ++++++++++++++++++++++++++------- crates/futures/tests/tests.rs | 18 ++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) mode change 100755 => 100644 crates/futures/tests/tests.rs diff --git a/crates/futures/src/lib.rs b/crates/futures/src/lib.rs index 4c6418f6..7bd5c9b2 100644 --- a/crates/futures/src/lib.rs +++ b/crates/futures/src/lib.rs @@ -108,6 +108,7 @@ extern crate js_sys; extern crate wasm_bindgen; use std::cell::{Cell, RefCell}; +use std::rc::Rc; use std::sync::Arc; use futures::executor::{self, Notify, Spawn}; @@ -336,15 +337,14 @@ fn _future_to_promise(future: Box>) -> P impl Notify for Package { fn notify(&self, _id: usize) { - match self.notified.replace(State::Notified) { - // we need to schedule polling to resume, so we do so - // immediately for now - State::Waiting(me) => Package::poll(&me), + let me = match self.notified.replace(State::Notified) { + // we need to schedule polling to resume, so keep going + State::Waiting(me) => me, // we were already notified, and were just notified again; // having now coalesced the notifications we return as it's // still someone else's job to process this - State::Notified => {} + State::Notified => return, // the future was previously being polled, and we've just // switched it to the "you're notified" state. We don't have @@ -353,8 +353,27 @@ fn _future_to_promise(future: Box>) -> P // continue polling. For us, though, there's nothing else to do, // so we bail out. // later see - State::Polling => {} - } + State::Polling => return, + }; + + // Use `Promise.then` on a resolved promise to place our execution + // onto the next turn of the microtask queue, enqueueing our poll + // operation. We don't currently poll immediately as it turns out + // `futures` crate adapters aren't compatible with it and it also + // helps avoid blowing the stack by accident. + // + // Note that the `Rc`/`RefCell` trick here is basically to just + // ensure that our `Closure` gets cleaned up appropriately. + let promise = Promise::resolve(&JsValue::undefined()); + let slot = Rc::new(RefCell::new(None)); + let slot2 = slot.clone(); + let closure = Closure::wrap(Box::new(move |_| { + let myself = slot2.borrow_mut().take(); + debug_assert!(myself.is_some()); + Package::poll(&me); + }) as Box); + promise.then(&closure); + *slot.borrow_mut() = Some(closure); } } } diff --git a/crates/futures/tests/tests.rs b/crates/futures/tests/tests.rs old mode 100755 new mode 100644 index 089003d5..2303d855 --- a/crates/futures/tests/tests.rs +++ b/crates/futures/tests/tests.rs @@ -7,6 +7,7 @@ extern crate wasm_bindgen_futures; extern crate wasm_bindgen_test; use futures::Future; +use futures::unsync::oneshot; use wasm_bindgen::prelude::*; use wasm_bindgen_futures::{future_to_promise, JsFuture}; use wasm_bindgen_test::*; @@ -48,3 +49,20 @@ fn error_future_is_rejected_promise() -> impl Future Ok(()) }) } + +#[wasm_bindgen] +extern { + fn setTimeout(c: &Closure); +} + +#[wasm_bindgen_test(async)] +fn oneshot_works() -> impl Future { + let (tx, rx) = oneshot::channel::(); + let mut tx = Some(tx); + let closure = Closure::wrap(Box::new(move || { + drop(tx.take().unwrap()); + }) as Box); + setTimeout(&closure); + closure.forget(); + rx.then(|_| Ok(())) +}