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: avoid manual poll impl for accounts events #1944

Merged
merged 3 commits into from Sep 29, 2020

Conversation

dignifiedquire
Copy link
Member

Fixes #1922

Copy link
Collaborator

@link2xt link2xt left a comment

Choose a reason for hiding this comment

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

This is a busy loop consuming 100% CPU now, as it never .awaits.

One of the solutions I can think of is to remove try_recv() completely and use select_all on a list of recv futures.

@dignifiedquire
Copy link
Member Author

thanks @link2xt should be good now

@link2xt
Copy link
Collaborator

link2xt commented Sep 27, 2020

But now if the first emitter is not finished yet, and only the second emitter has something, it will keep awaiting the first one without starting to await the second one. I don't think it can be solved without select.

src/accounts.rs Outdated
}
Err(async_std::sync::TryRecvError::Empty) => {}
for e in &self.0 {
if let Some(event) = e.emitter.recv().await {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will never start recv future for the second emitter, until the first emitter returns None.

@dignifiedquire
Copy link
Member Author

I don't think it can be solved without select.

I don't think select will work either, at least not naively, because there is a chance we will miss events, if two are ready at the same time

@link2xt
Copy link
Collaborator

link2xt commented Sep 27, 2020

I don't think select will work either, at least not naively, because there is a chance we will miss events, if two are ready at the same time

Then there is a stream version of select_all, which can be used to merge several event streams together: https://docs.rs/futures/0.3.5/futures/stream/fn.select_all.html

@dignifiedquire
Copy link
Member Author

@link2xt yes that was the right one, I think it works now

@dignifiedquire dignifiedquire changed the title fix: avoid manual poll impl fix: avoid manual poll impl for accounts events Sep 29, 2020
@dignifiedquire dignifiedquire merged commit 7786a4c into master Sep 29, 2020
@dignifiedquire dignifiedquire deleted the fix-accounts-events branch September 29, 2020 12:00
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.

accounts.EventEmitter.recv_poll does not setup waker
2 participants