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

Removes the events system from Network and ProtocolsHandler #685

Closed
tomaka opened this issue Nov 26, 2018 · 1 comment
Closed

Removes the events system from Network and ProtocolsHandler #685

tomaka opened this issue Nov 26, 2018 · 1 comment
Labels
difficulty:hard priority:important The changes needed are critical for libp2p, or are blocking another project

Comments

@tomaka
Copy link
Member

tomaka commented Nov 26, 2018

Right now each node gets assigned its own background task, and the Network asynchronously collects events that happen on these background tasks.

In order to avoid locking, what the Network knows is delayed compared to the actual state. For example the Network may think that we have a connection to a peer, while in fact this connection has already been closed a few hundred microseconds earlier.

Because of that, the only way the Network/Swarm/NetworkBehaviour and the ProtocolsHandler can communicate is by sending and receiving asynchronous events.

While this looks like a good idea, it makes writing correct APIs much more tedious. Instead of, for example, having an implementation of ProtocolsHandler with a method that does fn start_query() -> Result<(), Error> and calling it (which would be idiomatic Rust), you need to send a StartQuery event with some identifier, then process the fact that the handler can return a StartQueryError later. This also means that you need to handle the clean up in case the node gets its connection closed, and a timeout system.

In terms of efficiency, this also makes the API much more clone-heavy. Events, since they have to be 'static, need to hold clones of data, and cannot hold references.

Instead, I think that we should remove the inbound events entirely (events sent from the Network towards the ProtocolsHandler) and let the user of Network access the NodeHandler (and therefore ProtocolsHandler) by holding a Mutex when calling Network::peer(). However, in order to avoid logic errors, it should be possible to obtain a mutex to the NodeHandler only once we have processed all the pending outbound events coming from the handler of that node. This guarantees that the inside and the outside are successfully synchronized in terms of state.

In practice, a PeerConnected (in the raw_swarm module) should have a way to poll the events specific to that peer, and once all the events have been polled you obtain an object that Derefs to the NodeHandler.

The NetworkBehaviour should eventually be adjusted for that in the future, but for now I think it is enough to only modify the Network and not touch the NetworkBehaviour trait nor the API of Swarm.

@tomaka tomaka added priority:important The changes needed are critical for libp2p, or are blocking another project difficulty:moderate difficulty:hard and removed difficulty:moderate labels Nov 26, 2018
@romanb romanb changed the title Removes the events system from RawSwarm and ProtocolsHandler Removes the events system from Network and ProtocolsHandler Sep 10, 2019
@mxinden
Copy link
Member

mxinden commented May 8, 2022

I don't think a synchronous API enforced through a Mutex is a good path forward. I prefer us to stick with the current actor based approach.

I am closing here. In case folks feel strongly about the suggestion above, I am happy to reopen the issue.

@mxinden mxinden closed this as completed May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:hard priority:important The changes needed are critical for libp2p, or are blocking another project
Projects
None yet
Development

No branches or pull requests

2 participants