quic: returning ListenerClosed on connection closed internally

[jxs]

Current behaviour

In lighthouse while trying to shutdown the client, SwarmEvent::Listenerclosed was returned and logged:

CRIT Listener closed reason: Err(Custom { kind: Other, error: Transport(Custom { kind: Other, error: Right(EndpointDriverCrashed) }) }), addresses: [\"/ip4/127.0.0.1/udp/9001/quic-v1\", \"/ip4/141.95.85.227/udp/9001/quic-v1\"], service: libp2p

In the quic transport, when polling the Listener :

rust-libp2p/transports/quic/src/transport.rs

Lines 573 to 595 in 399eaa3

	match self.accept.poll_unpin(cx) {
	Poll::Ready(Some(connecting)) => {
	let endpoint = self.endpoint.clone();
	self.accept = async move { endpoint.accept().await }.boxed();
	let local_addr = socketaddr_to_multiaddr(&self.socket_addr(), self.version);
	let remote_addr = connecting.remote_address();
	let send_back_addr = socketaddr_to_multiaddr(&remote_addr, self.version);
	let event = TransportEvent::Incoming {
	upgrade: Connecting::new(connecting, self.handshake_timeout),
	local_addr,
	send_back_addr,
	listener_id: self.listener_id,
	};
	return Poll::Ready(Some(event));
	}
	Poll::Ready(None) => {
	self.close(Err(Error::EndpointDriverCrashed));
	continue;
	}
	Poll::Pending => {}
	};

as quinn (unlike tokio's TcpStream which cannot be closed) Accept Future impl is Option, it yields None when the endpoint has been closed and quic::Transport returns SwarmEvent::ListenerClosed.

This is different from the other transports, namely tcp which only returns SwarmEvent::ListenerClosed when Transport::remove_listener is called.

Possible Solution

Can we not send SwarmEvent::ListenerClosed when Accept returns None and just return Poll::Ready(None) to match other transports? @kpp what was the reason for considering Accept returning None a EndpointDriverCrashed ?

thanks

Version

• libp2p version (version number, commit, or branch):

Would you like to work on fixing this bug?

Yes

[thomaseizinger]

Can we not send SwarmEvent::ListenerClosed when Accept returns None and just return Poll::Ready(None) to match other transports?

I think this is what is currently happening, right? We call c.lose on the listener which queues a ListenerClosed event:

rust-libp2p/transports/quic/src/transport.rs

Lines 485 to 502 in 399eaa3

	/// Report the listener as closed in a [`TransportEvent::ListenerClosed`] and
	/// terminate the stream.
	fn close(&mut self, reason: Result<(), Error>) {
	if self.is_closed {
	return;
	}
	self.endpoint.close(From::from(0u32), &[]);
	self.pending_event = Some(TransportEvent::ListenerClosed {
	listener_id: self.listener_id,
	reason,
	});
	self.is_closed = true;
	// Wake the stream to deliver the last event.
	if let Some(waker) = self.close_listener_waker.take() {
	waker.wake();
	}
	}

This is then returned and eventually we return Poll::Ready(None):

rust-libp2p/transports/quic/src/transport.rs

Lines 563 to 568 in 399eaa3

	if let Some(event) = self.pending_event.take() {
	return Poll::Ready(Some(event));
	}
	if self.is_closed {
	return Poll::Ready(None);
	}

We might want to use a different reason though. I'd assume that Accept returning None means that the socket was closed gracefully and we should probably just pass Ok(()) to .close.

[jxs]

Can we not send SwarmEvent::ListenerClosed when Accept returns None and just return Poll::Ready(None) to match other transports?

I think this is what is currently happening, right? We call c.lose on the listener which queues a ListenerClosed event:
This is then returned and eventually we return Poll::Ready(None):

Yeah but we still send SwarmEvent::ListenerClosed before returning Poll::Ready(None) which doesn't happen in other transports. I'd expect SwarmEvent::ListenerClosed would only be returned when a user specifically asks the swarm to remove such listener.

[thomaseizinger]

But ListenerClosed specifically has a reason field that may be set to an error. That can't be user-requested then, can it?

It sounds to me like the other transports would need to be changed here and not quic. Or am I misunderstanding something?

[jxs]

But ListenerClosed specifically has a reason field that may be set to an error. That can't be user-requested then, can it?

indeed Thomas, thanks for pointing it out.

It sounds to me like the other transports would need to be changed here and not quic. Or am I misunderstanding something?

The thing is, other transports don't return None when the Endpoint has been closed, for example take the Stream implementation of tokio's TcpListener It never returns None. Probably only when dropped .

What you wrote regarding the reason makes sense to me, so if you agree I can submit a PR that changes Err(Error::EndpointDriverCrashed) to Ok(()) when calling self.close

[thomaseizinger]

What you wrote regarding the reason makes sense to me, so if you agree I can submit a PR that changes Err(Error::EndpointDriverCrashed) to Ok(()) when calling self.close

Yeah, I can't see any issue with that. Annoyingly, we don't know why the endpoint has been closed otherwise we could make it conditional on that.

[jxs]

submitted quinn-rs/quinn#1676