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(relay): wake the relay Listener on close #5491

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

stormshield-frb
Copy link
Contributor

@stormshield-frb stormshield-frb commented Jul 11, 2024

Description

When closing a relayed Listener manually, the TransportEvent::ListenerClosed generated by the relay::priv_client::Transport is never forwarded back up to the Swarm, causing the Swarm to never remove the corresponding listener and never emitting the SwarmEvent::ListenerClosed event.

This happens because, when stopping a relayed listener manually, the call to the close() function, is done outside the poll function, which mean nothing is triggering a wake up call to wake up the polling.

Unfortunately, even if the listeners (SelectAll) is always polled after a call to the close method, since SelectAll uses a FuturesUnordered internally, the poll does nothing. Indeed, the FuturesUnordered states that:

/// This structure is optimized to manage a large number of futures.
/// Futures managed by [`FuturesUnordered`] will only be polled when they
/// generate wake-up notifications. This reduces the required amount of work
/// needed to poll large numbers of futures.

Since means that when closing a relayed listener manually (calling swarm.remove_listener), it is never removed.

This PR fixes that by triggering a waker when calling the close function.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM, thanks François!

@jxs jxs added the send-it label Jul 29, 2024
@mergify mergify bot merged commit 41e2d5d into libp2p:master Jul 29, 2024
70 of 72 checks passed
@stormshield-frb stormshield-frb deleted the fix/close-relay-listener branch August 28, 2024 09:37
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
When closing a relayed `Listener` manually, the `TransportEvent::ListenerClosed` generated by the `relay::priv_client::Transport` is never forwarded back up to the `Swarm`, causing the `Swarm` to never remove the corresponding listener and never emitting the `SwarmEvent::ListenerClosed` event.

This happens  because, when stopping a relayed listener manually, the call to the [`close()` function](https://github.com/libp2p/rust-libp2p/blob/master/protocols/relay/src/priv_client/transport.rs#L324), is done outside the `poll` function, which mean nothing is triggering a wake up call to wake up the polling.

Unfortunately, even if the [`listeners` (`SelectAll`) is always polled](https://github.com/libp2p/rust-libp2p/blob/master/protocols/relay/src/priv_client/transport.rs#L241) after a call to the `close` method, since `SelectAll` uses a `FuturesUnordered` internally, the poll does nothing. Indeed, the `FuturesUnordered` states that:
```rust
/// This structure is optimized to manage a large number of futures.
/// Futures managed by [`FuturesUnordered`] will only be polled when they
/// generate wake-up notifications. This reduces the required amount of work
/// needed to poll large numbers of futures.
```

Since means that when closing a relayed listener manually (calling `swarm.remove_listener`), it is never removed.

This PR fixes that by triggering a `waker` when calling the `close` function.

Pull-Request: libp2p#5491.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants