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

[libp2p-swarm] Correct returned connections from notify_all. #1513

Merged
merged 2 commits into from
Mar 25, 2020

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Mar 24, 2020

This addresses a potential issue with notify_all in the swarm discovered on another review of the code triggered by #1509: If at least one connection was not ready (i.e. pending), only those (i.e. the pending) connections would be returned and considered on the next task wakeup, whereas those which were ready should also remain in the list of connections to notify on task wakeup and retry of notify_all. I admit that this deserves better testing, as does libp2p-swarm in general, which I hope to spend some more time on soon.

If at least one connection was not ready (i.e. pending), only
those (pending) connections would be returned and considered on the next
iteration, whereas those which were ready should also remain
in the list of connections to notify on retry of `notify_all`.
It seems unnecessary to use "poll all" -> "send all" semantics,
i.e. attempting an "atomic" broadcast. Rather, events send via
`notify_all` can be delivered as soon as possible, simplifying
the code further.
@romanb
Copy link
Contributor Author

romanb commented Mar 25, 2020

It occurred to me after a question from @twittner that it seems unnecessary to implement notify_all via "poll all" -> "send all" semantics. Rather, the event can be delivered to a handler as soon as the handler is ready, only tracking those remaining connections whose handlers are currently not ready. This further simplifies things and was done in c23075a.

@romanb romanb merged commit 28ea62d into libp2p:master Mar 25, 2020
@romanb romanb deleted the swarm-notify-all-alive branch March 25, 2020 12:53
folex added a commit to fluencelabs/rust-libp2p that referenced this pull request Mar 30, 2020
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.

3 participants