Discussion: Closing Listeners

[elenaf9]

Hi,
I'd like to add some thoughts on how ExpandedSwarm.remove_listener is implemented, because I think it is a bit confusing.
I personally would expect remove_listener to work analogous to how start_listening is behaving.
For start_listening, a new Listener is created and SwarmEvent::NewListenAddr are reported. This allows to verify that there is actually a new active listener.
For remove_listener, I would expect that the Listener is closed and that there will be an event SwarmEvent::ListenerClosed and SwarmEvent::ExpiredListenAddr. This is currently not the case.

If I understand the current implementation correctly, it works like this:

• start_listening creates a new Listener by calling Transport.listen_on
• the listener is added to a list of Listeners, that are polled continuously for ListenerEvents
• if there are events for that listener, e.g. NewAddress, it is bubbled up and a SwarmEvent is created for it
• ListenersEvent::Closed only occurs if polling the Listener returns None.
• on remove_listener, the listener is removed from the list, hence not polled anymore. No event is created.
• transport implementations implement Drop for a Listener, to have a way of e.g. freeing allocated ports.

While this allows to implement all the necessary logic, I personally think it is a bit in-transparent.

I'd like to suggest that instead the Transport::Listener should be required to implement a trait:

pub trait StopListening {
   fn stop_listening();
} 

Instead of dropping the listener, the Network would call this method.
The listener can then e.g. set a local flag and return None in poll_next(), which would result in a ListenersEvent::Closed.
Optionally the listener would this way also have the chance of emitting ListenersEvent::AddressExpired for its addresses.

Another thought is that a stop_listening method would maybe also make sense for the Transport trait:

fn stop_listener(self, Self::Listener) {}

This would also allow the transport to react to it.
For example in the memory transport, ports are registered in the HUB in the Transport.listen_on method, and the de-registering happens in Listener.drop(); the HUB has to be shared in a static item. With a stop_listener method, the HUB Mutex could be a property of the MemoryTransport and the port could be de-registered there.

Btw another question in this context: the methods of Transport consume self, because according to a note:

This has been designed so that you would implement this trait on &Foo or &mut Foo instead of directly on Foo

but existing transport implementations like the MemoryTransport and GenTcpConfig both implement it directly. Could you point out to me why this is the case?

---

What are your thoughts on this matter?

[mxinden]

Lots to unpack here. Let me try to answer some of it.

For remove_listener, I would expect that the Listener is closed and that there will be an event SwarmEvent::ListenerClosed and SwarmEvent::ExpiredListenAddr. This is currently not the case.

Yes, that would be desirable. I am happy to help design such change, though I don't think driving it myself is a current priority for me.

Related to this is the past graceful shutdown effort. This is harder than it seems, though I haven't looked deep enough into Roman's work to have a solid opinion. That said, I don't think this should demotivate, but instead help to gain a greater overview.

---

or example in the memory transport, ports are registered in the HUB in the Transport.listen_on method, and the de-registering happens in Listener.drop(); the HUB has to be shared in a static item. With a stop_listener method, the HUB Mutex could be a property of the MemoryTransport and the port could be de-registered there.

I don't quite see the concrete benefit for MemoryTransport here, given that both performance and complexity don't play an important role.

---

Another thought I had recently - still very vague- looking into #1334, is to remove the Listener object. The notion of a Listener (not the notion of listening on an address) seems overly TCP specific, e.g. not relevant for QUIC. One would still call Transport::listen_on, though new connections would be handed over via Transport instead of Listener.

---

Btw another question in this context: the methods of Transport consume self, because according to a note:

This has been designed so that you would implement this trait on &Foo or &mut Foo instead of directly on Foo

but existing transport implementations like the MemoryTransport and GenTcpConfig both implement it directly. Could you point out to me why this is the case?

Good catch! I don't know to be honest. Without much thought I would guess this comment is a relic of the past. Maybe @tomaka knows more?

[elenaf9]

I'd like to revive this discussion, because it is relevant for the implementation of AutoNAT.
Because closing a listener via Swarm::remove_listener does not create a ListenersEvent::Closed/ ListenersEvent::AddressExpired, the NetworkBehaviour(s) never get informed that a listening address is not valid anymore (i.g. inject_listener_closed/ inject_expired_listen_addr is never called). In my current draft for AutoNAT, the behaviour maintains a list of its listening addresses. These are send to the remote during the AutoNAT handshake, so that the remote can try to dial them. With the current implementation of Swarm::remove_listener, the listening addresses associated with the manually closed listener can not be removed from the list. This results in the remote peer attempting to dial addresses that are guaranteed to fail, which should be avoided.

Yes, that would be desirable. I am happy to help design such change, though I don't think driving it myself is a current priority for me.

I would tackle this issue, but I not too familiar with every related to the Listeners, hence I am not really sure what the best design would be. An idea would be what I suggested in the first commend above, to add a stop_listening to either the Listener, or the Transport, but I can not really judge whether this makes sense or not.

Another thought I had recently - still very vague- looking into #1334, is to remove the Listener object. The notion of a Listener (not the notion of listening on an address) seems overly TCP specific, e.g. not relevant for QUIC. One would still call Transport::listen_on, though new connections would be handed over via Transport instead of Listener.

@mxinden Is this still an desired change? And if yes, could you expand on the idea a bit more?

[mxinden]

With the current implementation of Swarm::remove_listener, the listening addresses associated with the manually closed listener can not be removed from the list. This results in the remote peer attempting to dial addresses that are guaranteed to fail, which should be avoided.

While maybe not an urgent problem for a first version of AutoNAT, I agree that this is worth fixing.

Yes, that would be desirable. I am happy to help design such change, though I don't think driving it myself is a current priority for me.

I would tackle this issue, but I not too familiar with every related to the Listeners, hence I am not really sure what the best design would be. An idea would be what I suggested in the first commend above, to add a stop_listening to either the Listener, or the Transport, but I can not really judge whether this makes sense or not.

Off the top of my head the idea of a stop_listening on Listener sounds reasonable to me, though I would need to explore this myself before being able to comment further. Feel free to propose a small proof-of-concept in case you are interested.

Another thought I had recently - still very vague- looking into #1334, is to remove the Listener object. The notion of a Listener (not the notion of listening on an address) seems overly TCP specific, e.g. not relevant for QUIC. One would still call Transport::listen_on, though new connections would be handed over via Transport instead of Listener.

@mxinden Is this still an desired change? And if yes, could you expand on the idea a bit more?

It is still an idea I would like to explore, though not high up on my priority list. I can likely give a better response once I have given #2159 an in-depth review. //CC @kpp and @dvc94ch as you are working on #2159.

[elenaf9]

Off the top of my head the idea of a stop_listening on Listener sounds reasonable to me, though I would need to explore this myself before being able to comment further. Feel free to propose a small proof-of-concept in case you are interested.

I have created a Draft-PR in #2261. I decided against adding a StopListening trait bound for Transport::Listener, because it didn't really make sense to me after looking at it again, especially because there would be no guarantee that the Listener actually closes its Stream.
Instead, the ListenerStream/ connection::Listener that wraps the Transport::Listener directly implements the necessary logic.

[mxinden]

For others and for the record, #2261 is merged now, addressing the listener closing part of this discussion.