Skip to content

Commit d32b6a9

Browse files
committed
Update futures implementation to not destroy callbacks
JS engines guarantee that at least one of our `then` callbacks are invoked, so that means if we destroy them prematurely they're guaranteed to log an exception to the console! Instead to prevent exceptions from happening tweak how the completion callbacks for JS futures are managed and ensure that the closures stay alive until they're invoked later. Closes #1637
1 parent 604c036 commit d32b6a9

File tree

2 files changed

+81
-71
lines changed

2 files changed

+81
-71
lines changed

crates/futures/src/futures_0_3.rs

Lines changed: 37 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ use std::collections::VecDeque;
33
use std::fmt;
44
use std::future::Future;
55
use std::pin::Pin;
6+
use std::rc::Rc;
67
use std::sync::Arc;
78
use std::task::{Context, Poll};
89

@@ -24,10 +25,7 @@ use wasm_bindgen::prelude::*;
2425
///
2526
/// Currently this type is constructed with `JsFuture::from`.
2627
pub struct JsFuture {
27-
resolved: oneshot::Receiver<JsValue>,
28-
rejected: oneshot::Receiver<JsValue>,
29-
_cb_resolve: Closure<dyn FnMut(JsValue)>,
30-
_cb_reject: Closure<dyn FnMut(JsValue)>,
28+
rx: oneshot::Receiver<Result<JsValue, JsValue>>,
3129
}
3230

3331
impl fmt::Debug for JsFuture {
@@ -38,31 +36,37 @@ impl fmt::Debug for JsFuture {
3836

3937
impl From<Promise> for JsFuture {
4038
fn from(js: Promise) -> JsFuture {
41-
// Use the `then` method to schedule two callbacks, one for the
42-
// resolved value and one for the rejected value. These two callbacks
43-
// will be connected to oneshot channels which feed back into our
44-
// future.
45-
//
46-
// This may not be the speediest option today but it should work!
47-
let (tx1, rx1) = oneshot::channel();
48-
49-
let cb_resolve = Closure::once(move |val| {
50-
tx1.send(val).unwrap_throw();
51-
});
52-
53-
let (tx2, rx2) = oneshot::channel();
54-
55-
let cb_reject = Closure::once(move |val| {
56-
tx2.send(val).unwrap_throw();
57-
});
58-
59-
js.then2(&cb_resolve, &cb_reject);
60-
61-
JsFuture {
62-
resolved: rx1,
63-
rejected: rx2,
64-
_cb_resolve: cb_resolve,
65-
_cb_reject: cb_reject,
39+
// See comments in `src/lib.rs` for why we're using one self-contained
40+
// callback here.
41+
let (tx, rx) = oneshot::channel();
42+
let state = Rc::new(RefCell::new(None));
43+
let state2 = state.clone();
44+
let resolve = Closure::once(move |val| finish(&state2, Ok(val)));
45+
let state2 = state.clone();
46+
let reject = Closure::once(move |val| finish(&state2, Err(val)));
47+
48+
js.then2(&resolve, &reject);
49+
*state.borrow_mut() = Some((tx, resolve, reject));
50+
51+
return JsFuture { rx };
52+
53+
fn finish(
54+
state: &RefCell<
55+
Option<(
56+
oneshot::Sender<Result<JsValue, JsValue>>,
57+
Closure<dyn FnMut(JsValue)>,
58+
Closure<dyn FnMut(JsValue)>,
59+
)>,
60+
>,
61+
val: Result<JsValue, JsValue>,
62+
) {
63+
match state.borrow_mut().take() {
64+
// We don't have any guarantee that anyone's still listening at this
65+
// point (the Rust `JsFuture` could have been dropped) so simply
66+
// ignore any errors here.
67+
Some((tx, _, _)) => drop(tx.send(val)),
68+
None => wasm_bindgen::throw_str("cannot finish twice"),
69+
}
6670
}
6771
}
6872
}
@@ -71,16 +75,11 @@ impl Future for JsFuture {
7175
type Output = Result<JsValue, JsValue>;
7276

7377
fn poll(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
74-
// Test if either our resolved or rejected side is finished yet.
75-
if let Poll::Ready(val) = self.resolved.poll_unpin(cx) {
76-
return Poll::Ready(Ok(val.unwrap_throw()));
77-
}
78-
79-
if let Poll::Ready(val) = self.rejected.poll_unpin(cx) {
80-
return Poll::Ready(Err(val.unwrap_throw()));
78+
match self.rx.poll_unpin(cx) {
79+
Poll::Pending => Poll::Pending,
80+
Poll::Ready(Ok(val)) => Poll::Ready(val),
81+
Poll::Ready(Err(_)) => wasm_bindgen::throw_str("cannot cancel"),
8182
}
82-
83-
Poll::Pending
8483
}
8584
}
8685

crates/futures/src/lib.rs

Lines changed: 44 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,7 @@ use wasm_bindgen::prelude::*;
128128
///
129129
/// Currently this type is constructed with `JsFuture::from`.
130130
pub struct JsFuture {
131-
resolved: oneshot::Receiver<JsValue>,
132-
rejected: oneshot::Receiver<JsValue>,
133-
callbacks: Option<(Closure<dyn FnMut(JsValue)>, Closure<dyn FnMut(JsValue)>)>,
131+
rx: oneshot::Receiver<Result<JsValue, JsValue>>,
134132
}
135133

136134
impl fmt::Debug for JsFuture {
@@ -142,28 +140,49 @@ impl fmt::Debug for JsFuture {
142140
impl From<Promise> for JsFuture {
143141
fn from(js: Promise) -> JsFuture {
144142
// Use the `then` method to schedule two callbacks, one for the
145-
// resolved value and one for the rejected value. These two callbacks
146-
// will be connected to oneshot channels which feed back into our
147-
// future.
143+
// resolved value and one for the rejected value. We're currently
144+
// assuming that JS engines will unconditionally invoke precisely one of
145+
// these callbacks, no matter what.
148146
//
149-
// This may not be the speediest option today but it should work!
150-
let (tx1, rx1) = oneshot::channel();
151-
let (tx2, rx2) = oneshot::channel();
152-
let mut tx1 = Some(tx1);
153-
let resolve = Closure::wrap(Box::new(move |val| {
154-
drop(tx1.take().unwrap().send(val));
155-
}) as Box<dyn FnMut(_)>);
156-
let mut tx2 = Some(tx2);
157-
let reject = Closure::wrap(Box::new(move |val| {
158-
drop(tx2.take().unwrap().send(val));
159-
}) as Box<dyn FnMut(_)>);
147+
// Ideally we'd have a way to cancel the callbacks getting invoked and
148+
// free up state ourselves when this `JsFuture` is dropped. We don't
149+
// have that, though, and one of the callbacks is likely always going to
150+
// be invoked.
151+
//
152+
// As a result we need to make sure that no matter when the callbacks
153+
// are invoked they are valid to be called at any time, which means they
154+
// have to be self-contained. Through the `Closure::once` and some
155+
// `Rc`-trickery we can arrange for both instances of `Closure`, and the
156+
// `Rc`, to all be destroyed once the first one is called.
157+
let (tx, rx) = oneshot::channel();
158+
let state = Rc::new(RefCell::new(None));
159+
let state2 = state.clone();
160+
let resolve = Closure::once(move |val| finish(&state2, Ok(val)));
161+
let state2 = state.clone();
162+
let reject = Closure::once(move |val| finish(&state2, Err(val)));
160163

161164
js.then2(&resolve, &reject);
165+
*state.borrow_mut() = Some((tx, resolve, reject));
166+
167+
return JsFuture { rx };
162168

163-
JsFuture {
164-
resolved: rx1,
165-
rejected: rx2,
166-
callbacks: Some((resolve, reject)),
169+
fn finish(
170+
state: &RefCell<
171+
Option<(
172+
oneshot::Sender<Result<JsValue, JsValue>>,
173+
Closure<dyn FnMut(JsValue)>,
174+
Closure<dyn FnMut(JsValue)>,
175+
)>,
176+
>,
177+
val: Result<JsValue, JsValue>,
178+
) {
179+
match state.borrow_mut().take() {
180+
// We don't have any guarantee that anyone's still listening at this
181+
// point (the Rust `JsFuture` could have been dropped) so simply
182+
// ignore any errors here.
183+
Some((tx, _, _)) => drop(tx.send(val)),
184+
None => wasm_bindgen::throw_str("cannot finish twice"),
185+
}
167186
}
168187
}
169188
}
@@ -173,19 +192,11 @@ impl Future for JsFuture {
173192
type Error = JsValue;
174193

175194
fn poll(&mut self) -> Poll<JsValue, JsValue> {
176-
// Test if either our resolved or rejected side is finished yet. Note
177-
// that they will return errors if they're disconnected which can't
178-
// happen until we drop the `callbacks` field, which doesn't happen
179-
// till we're done, so we dont need to handle that.
180-
if let Ok(Async::Ready(val)) = self.resolved.poll() {
181-
drop(self.callbacks.take());
182-
return Ok(val.into());
183-
}
184-
if let Ok(Async::Ready(val)) = self.rejected.poll() {
185-
drop(self.callbacks.take());
186-
return Err(val);
195+
match self.rx.poll() {
196+
Ok(Async::Ready(val)) => val.map(Async::Ready),
197+
Ok(Async::NotReady) => Ok(Async::NotReady),
198+
Err(_) => wasm_bindgen::throw_str("cannot cancel"),
187199
}
188-
Ok(Async::NotReady)
189200
}
190201
}
191202

0 commit comments

Comments
 (0)