Fix RunLoop behavior as an async Stream #216
Conversation
|
Added |
|
Consider replacing "on state change" wakers with callbacks, maybe pass struct with details about event. |
src/runtime/runloop.rs
Outdated
| @@ -63,6 +63,7 @@ where | |||
| /// Poll this runtime without exiting. Discards any value returned from the | |||
| /// root function. The future yields in between revisions and is woken on | |||
| /// state changes. | |||
| #[deprecated(note = "Blocks indefinitely. Use BindStream instead.")] | |||
There was a problem hiding this comment.
Is it not possible to retrofit this fix onto the RunLoop type?
There was a problem hiding this comment.
It doesn't make sense to change waker via set_state_change_waker between polls because mpsc receiver doesn't have this ability and may cause some unexpected behaviour either way. To maintain API i made BindStream, and deprecated RunLoop::run_on_state_changes (sadly you cannot deprecate trait implementation)
There was a problem hiding this comment.
I appreciate your attempt to maintain compatibility, but there are very few users of the crate outside of the tree :D. It's fine to change/break the existing RunLoop API as long as the dom examples keep working.
Once you've made the changes and are sure they're breaking, you can run cargo ofl versions to bump the minor version of the moxie crate, and the tool will handle updating the other crates in the repo.
There was a problem hiding this comment.
Another option would be to remove the async integration from RunLoop entirely, although I'd rather avoid adding more types than necessary. If RunLoop and BindStream can be the same type and still support the requestAnimationFrame setup, I think they should be.
src/runtime/bind_stream.rs
Outdated
| pub struct BindStream<Root> { | ||
| inner: Runtime, | ||
| root: Root, | ||
| changes_receiver: mpsc::Receiver<()>, |
There was a problem hiding this comment.
Could this be something like Arc<AtomicBool> instead? If multiple state updates occur before the stream is polled again we shouldn't run multiple times, right?
There was a problem hiding this comment.
This was my first attempt. The problem was about wakers somehow getting invalidated after storing them, before some change occurs, therefore calling noop on wake. I looked up example of Stream implementation that was just a wrapper of another stream/future. So i scrapped my initial attempt and just wrapped mpsc receiver as it implements Stream trait. And it worked! The next logical step would be to see how mpsc implemented and use that... buuuut i don't see if there can be any significant performance gain while sacrificing some maintainability and adding more code. Either way there will be some indirection because wrapping callback inside a waker, to send message through channel and give receiver control over waker or to update atomic bool then calling wrapped wake. And we use bounded mpsc channel practically with a buffer for one message, ignoring buffer full error and not sending a new message (should add test for this)
There was a problem hiding this comment.
And we use bounded mpsc channel practically with a buffer for one message, ignoring buffer full error and not sending a new message (should add test for this)
Ah, I missed this in my first pass. This makes sense and is fine as long as it's documented.
|
Sorry for the long delays in reply here! |
|
Closed in favor of #223 |
Fix #215
src\runtime\runloop.rs:115waker is a wrapped regular task wakersrc\runtime\runloop.rs:183waker is a wrapped noopRunLoopcan be stalled indefinitely even if some changes exists.Fails runloop_in_pool so far.