Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RunLoop behavior as an async Stream #216

Closed
wants to merge 0 commits into from

Conversation

zetanumbers
Copy link
Contributor

@zetanumbers zetanumbers commented Nov 28, 2020

Fix #215
src\runtime\runloop.rs:115 waker is a wrapped regular task waker
src\runtime\runloop.rs:183 waker is a wrapped noop
RunLoop can be stalled indefinitely even if some changes exists.
Fails runloop_in_pool so far.

@zetanumbers
Copy link
Contributor Author

Added BindStream. Deprecated use of RunLoop as asynchronous stream.

@zetanumbers
Copy link
Contributor Author

Consider replacing "on state change" wakers with callbacks, maybe pass struct with details about event.

@@ -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.")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not possible to retrofit this fix onto the RunLoop type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pub struct BindStream<Root> {
inner: Runtime,
root: Root,
changes_receiver: mpsc::Receiver<()>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@anp
Copy link
Owner

anp commented Dec 6, 2020

Sorry for the long delays in reply here!

@zetanumbers
Copy link
Contributor Author

Closed in favor of #223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RunLoop::run_on_state_changes() shouldn't call run_once unless a state change occurred
2 participants