swarm/: include ListenerId in SwarmEvents

[elenaf9]

This PR drafts a change that I'd like to suggest:
I think it would be useful if in case of SwarmEvents associated with a listener (e.g. SwarmEvent::NewListenAddr), the event would include the id of the associated listener.
It allows to map the event back to a previously called swarm.listen_on(). This is especially useful if swarm.listen_on() was called with an zeroed Multiaddress (e.g. /ip4/0.0.0.0/tcp/0), and the actual new address should be mapped back to the newly created listener, without blocking.
Since this information is already in the NetworkEvent that the ExpandedSwarm receives, I don't see a reason to not include it, especially since the user already has to deal with ListenerIds.

[elenaf9]

Note: one of the the relay tests (test inactive_connection_timeout) failed, even though my changes should not have affected any logic related to it. The test passes if I test it locally, I'd guess it's a flaky test that only occasionally fails, but from a first quick look I can't find the reason for this.

[thomaseizinger]

Implementation looks good to me! I actually have a direct use for this in the tests for the rendezvous protocol! :)

When glancing over the code, I noticed that the relay tests might benefit from this aswell.

[elenaf9]

When glancing over the code, I noticed that the relay tests might benefit from this aswell.

@thomaseizinger do you mean by doing something like this?:

let new_listener = dst_swarm.listen_on(dst_addr_via_relay.clone()).unwrap();
...
match dst_swarm.select_next_some().await {
    SwarmEvent::NewListenAddr { address, listener_id} if listener_id == new_listener {
        assert_eq!(address, dst_addr_via_relay);
        break;
   }
...

I could add that, just wanted to know first whether the change in this PR is generally desired.

What do you think @mxinden?

[thomaseizinger]

Yep, that is what I was thinking.

To be fair, for this case it doesn't matter much because comparing the address also works :)

[mxinden]

Exposing ListenerId on NewListenAddr, ExpiredListenAddr and ListenerClosed sounds good to me. Thanks for the patch @elenaf9!

It allows to map the event back to a previously called swarm.listen_on(). This is especially useful if swarm.listen_on() was called with an zeroed Multiaddress (e.g. /ip4/0.0.0.0/tcp/0), and the actual new address should be mapped back to the newly created listener, without blocking.

Just a note: a single listen_on might cause multiple NewListenAddr events.

Would you mind adding a note to Swarm::listen_on and Swarm::remove_listener mentioning that they cause one or more NewListenAddr or ExpiredListenAddr events with the same ListenerId. I could imagine this being helpful for newcomers.

Also, could you add an entry to swarm/CHANGELOG.md?

[mxinden]

Suggested change

	/// The new address that is being listened on.
	/// The expired address.

[elenaf9]

Would you mind adding a note to Swarm::listen_on and Swarm::remove_listener mentioning that they cause one or more NewListenAddr or ExpiredListenAddr events with the same ListenerId. I could imagine this being helpful for newcomers.

Sounds good, I'll do that. Just one note (related to Discussion #2113): If I'm not mistaken Swarm::remove_listener currently does not create any SwarmEvent::ExpiredListenAddr, hence adding such a note to this method would be misleading? But I'll add it for Swarm::listen_on.

[mxinden]

Sounds good, I'll do that. Just one note (related to Discussion #2113): If I'm not mistaken Swarm::remove_listener currently does not create any SwarmEvent::ExpiredListenAddr, hence adding such a note to this method would be misleading? But I'll add it for Swarm::listen_on.

Right. My bad. Thanks @elenaf9!

[mxinden]

Thanks @elenaf9!

Just a couple of nit picks on naming in relay/tests/lib to be consistent with the other variable names. Other than that this is good to go from my side.