Adding in Futures 0.3 support to wasm-bindgen-futures #1507
Conversation
|
I tried to test this, but I keep running into #1501 (comment) and I wasn't able to compile any of the examples. |
There was a problem hiding this comment.
Code-wise looks great to me, thanks @Pauan! I had some questions inline, but I might recommend renaming nightly to std-futures or something like that? It'll only be nightly very temporarily :)
crates/futures/src/nightly.rs
Outdated
| F: Future<Output = ()> + 'static, | ||
| { | ||
| struct Task { | ||
| future: Mutex<Option<Pin<Box<dyn Future<Output = ()> + 'static>>>>, |
There was a problem hiding this comment.
Since the Future here is fundamentally non-Send, is the Mutex/Atomic* required or could they be replaced with non-threadsafe variants?
There was a problem hiding this comment.
Also, orthogonally, I wasn't quite sure why I initially saw Option here, but is the idea that you can drop the future at any time but there may still be lingering handles to the Task for awhile longer while events are processed throughout the system?
There was a problem hiding this comment.
The atomic stuff isn't needed, it's just a holdover from when I tried to make it thread safe (and then later gave up).
The reason for the Option is because the Future might return Poll::Ready and also call waker.wake(). That would end up causing an infinite loop, so the Option lets us avoid that.
Sure, that's technically against the Future contract, but I like to make the code rock solid so it can survive even bad Futures.
It also has the benefit that it can drop the Future immediately when it returns Poll::Ready (rather than at some potentially far-future time when the Task is eventually dropped).
Because the Waker contains an Arc<Task>, it's theoretically possible that the Future will never get dropped (if someplace somewhere keeps a hold of the Waker). So the Option lets us fix that.
crates/futures/src/nightly.rs
Outdated
| impl ArcWake for Task { | ||
| fn wake_by_ref(arc_self: &Arc<Self>) { | ||
| // TODO can this be more relaxed ? | ||
| if !arc_self.is_queued.swap(true, Ordering::SeqCst) { |
There was a problem hiding this comment.
Could this be swapped for if swap(true) { return } to avoid indentation where we can?
crates/futures/src/nightly.rs
Outdated
|
|
||
| impl ArcWake for Task { | ||
| fn wake_by_ref(arc_self: &Arc<Self>) { | ||
| // TODO can this be more relaxed ? |
There was a problem hiding this comment.
Technically the ordering doesn't matter at all today in the sense that all orderings are equivalent when you're dealing with one thread of code. For multithreaded code wasm has no concept of orderings, so we'll want to stick SeqCst everywhere
There was a problem hiding this comment.
That's good to know about multi-threaded wasm.
crates/futures/src/lib.rs
Outdated
| rejected: oneshot::Receiver<JsValue>, | ||
| callbacks: Option<(Closure<FnMut(JsValue)>, Closure<FnMut(JsValue)>)>, | ||
| } | ||
| #[cfg(not(feature = "nightly"))] |
There was a problem hiding this comment.
I think that we'll probably want to have the standard library futures support in its own module. We don't want enabling the nightly feature to break this crate (features should be additive in terms of functionality) for existing users. That does mean that if nightly is enabled it'll pull in two implementations of futures, but I think that's ok since the linker will eliminate them if they're not used.
|
Okay, I addressed all the feedback. |
|
👍 nice! |
I haven't tested this yet, but it probably works.
I put everything behind the
nightlyfeature. I'm open to changing the name.Unfortunately I can't really add unit tests, because the
wasm-bindgen-testcrate requires Futures 0.1