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

Remove usage of ConnectionHandlerEvent::Close from libp2p_relay::client #4717

Closed
thomaseizinger opened this issue Oct 24, 2023 · 2 comments · Fixed by #4745
Closed

Remove usage of ConnectionHandlerEvent::Close from libp2p_relay::client #4717

thomaseizinger opened this issue Oct 24, 2023 · 2 comments · Fixed by #4745
Assignees

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Oct 24, 2023

Mostly documenting this for myself here but happy to have other people join in.

I looked into this today and it is quite tricky. I Basically, we will need to remove all the FatalUpgradeErrors. Additionally, we'll have to change how we handle errors in general. For example, at the moment, failing to establish a connection a circuit fails the Transport::Dial with a generic Connect error. The real error reason is emitted as part of libp2p::relay::client::Event::OutboundCircuitReqFailed. That doesn't really make a whole lot of sense.

As a first step, I think we should change this bit and actually send the error that happens as part of the upgrade through the channel to the transport, make it the error of Transport::Dial and remove the libp2p::relay::client::Event::OutboundCircuitReqFailed variant.

Similarly, I think a failed reservation should fail the listener in the transport and report the error in ListenerClosed::reason.

Lastly, InboundCircuitReqDenied and InboundCircuitReqDenyFailed are implementation details that we don't need to expose to the user at all. Technically, we could avoid these altogether by off-loading these to the protocol selection layer. If we don't have a reservation on a connection, we could just not allow the /hop protocol on inbound connections.

@thomaseizinger
Copy link
Contributor Author

cc @mxinden Keen to get your input on this.

@thomaseizinger thomaseizinger self-assigned this Oct 27, 2023
@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Oct 27, 2023

So in the hole-punching tests, we are actually encountering this problem: https://github.com/libp2p/rust-libp2p/actions/runs/6660850619/job/18102703697?pr=4739#step:5:579

To better debug those errors, I am looking into this.

Overall, I think it is much cleaner if the listener fails than emitting an error somewhere. The user activates the process with Swarm::listen_on so that is where we should emit the error.

@mergify mergify bot closed this as completed in #4745 Oct 31, 2023
mergify bot pushed a commit that referenced this issue Oct 31, 2023
To make a reservation with a relay, a user calls `Swarm::listen_on` with an address of the relay, suffixed with a `/p2pcircuit` protocol. Similarly, to establish a circuit to another peer, a user needs to call `Swarm::dial` with such an address. Upon success, the `Swarm` then issues a `SwarmEvent::NewListenAddr` event in case of a successful reservation or a `SwarmEvent::ConnectionEstablished` in case of a successful connect.

The story is different for errors. Somewhat counterintuitively, the actual reason of an error during these operations are only reported as `relay::Event`s without a direct correlation to the user's `Swarm::listen_on` or `Swarm::dial` calls.

With this PR, we send these errors back "into" the `Transport` and report them as `SwarmEvent::ListenerClosed` or `SwarmEvent::OutgoingConnectionError`. This is conceptually more correct. Additionally, by sending these errors back to the transport, we no longer use `ConnectionHandlerEvent::Close` which entirely closes the underlying relay connection. In case the connection is not used for something else, it will be closed by the keep-alive algorithm.

Resolves: #4717.
Related: #3591.
Related: #4718.

Pull-Request: #4745.
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 a pull request may close this issue.

1 participant