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 potential bug when polling the Swarm #1509

Closed
wants to merge 1 commit into from

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Mar 24, 2020

In the situation where this.network.poll returns Poll::Ready(_) and then this.behaviour.poll returns Poll::Ready(NetworkBehaviourAction::NotifyHandler { .. }), we would return Poll::Pending.

That is wrong, as nothing in the stack has registered the Waker to wake it up later. In other words, the task would then get frozen forever.

Same problem if this.network.poll returns Ready and we have an event buffered up that we fail to deliver.

@tomaka tomaka requested a review from romanb March 24, 2020 12:24
@romanb
Copy link
Contributor

romanb commented Mar 24, 2020

That is wrong, as nothing in the stack has registered the Waker to wake it up later. In other words, the task would then get frozen forever.

I'm not sure I follow, I will need to take a closer look later, but all the occurrences of return Poll::Pending that you removed here are in the context of notify_one / notify_any / notify_all having returned Some, which only happens when the current task is registered to be woken up because at least one connection returned Poll::Pending from poll_ready_notify_handler. At least that is the intention and how I documented it.

@tomaka
Copy link
Member Author

tomaka commented Mar 24, 2020

Oh ok, I see. I admit that it didn't occur to me that notify was polling, as I still had the former design with the unbounded channels in my head.

@romanb
Copy link
Contributor

romanb commented Mar 24, 2020

No problem, I actually found an unrelated issue on re-reviewing the code: #1513.

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.

2 participants