chore: closing ping streams

[gabrielmer]

Description

We're opening ping streams every 2 minutes but not closing them. This means we get an ever increasing amount of stale streams. Therefore, closing the stream after the ping.

Changes

• close ping stream after usage

Testing

Tested it by calling

nwaku/waku/node/peer_manager/peer_manager.nim

Lines 655 to 671 in a37c9ba

	proc connectedPeers*(pm: PeerManager, protocol: string): (seq[PeerId], seq[PeerId]) =
	## Returns the peerIds of physical connections (in and out)
	## containing at least one stream with the given protocol.
	var inPeers: seq[PeerId]
	var outPeers: seq[PeerId]
	for peerId, muxers in pm.switch.connManager.getConnections():
	for peerConn in muxers:
	let streams = peerConn.getStreams()
	if streams.anyIt(it.protocol == protocol):
	if peerConn.connection.transportDir == Direction.In:
	inPeers.add(peerId)
	elif peerConn.connection.transportDir == Direction.Out:
	outPeers.add(peerId)
	return (inPeers, outPeers)

and

nwaku/waku/node/peer_manager/peer_manager.nim

Lines 673 to 685 in a37c9ba

	proc getNumStreams*(pm: PeerManager, protocol: string): (int, int) =
	var
	numStreamsIn = 0
	numStreamsOut = 0
	for peerId, muxers in pm.switch.connManager.getConnections():
	for peerConn in muxers:
	for stream in peerConn.getStreams():
	if stream.protocol == protocol:
	if stream.dir == Direction.In:
	numStreamsIn += 1
	elif stream.dir == Direction.Out:
	numStreamsOut += 1
	return (numStreamsIn, numStreamsOut)

inside keepaliveLoop() and logging the outputs

The conclusions were:

• without calling conn.close(), the output of getNumStreams() grew indefinitely
• calling conn.close(), the output of getNumStreams() varied but was stable
• calling conn.close()did not affect the output of connectedPeers(), which means that indeed it didn't affect the physical connections

Issue

closes #2462

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2692-rln-v1

Built from 7a6c35f

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2692-rln-v2

Built from 7a6c35f

[NagyZoltanPeter]

Good catch. Isn't this feature is disabled by default?

[gabrielmer]

Good catch. Isn't this feature is disabled by default?

Yes! So the feature is disabled by default, but in nwaku-compose we do start the node with keep-alive=true

[AlbertoSoutullo]

Yes! So the feature is disabled by default, but in nwaku-compose we do start the node with keep-alive=true

Wait, one question so I can understand this. Are you saying that this behavior were happening only if we had the keep-alive flag set to True?

[alrevuelta]

lgtm.

Was wondering why we were not seeing this in waku_streams_peers but just realized we only consider waku protocols (ping is excluded).

good catch

[gabrielmer]

Wait, one question so I can understand this. Are you saying that this behavior were happening only if we had the keep-alive flag set to True?

Yes, if keep-alive is set to false then we wouldn't open those ping connections every 2 minutes that make the open connections to grow indefinitely

[Ivansete-status]

LGTM! Thanks! 💯
As a possible future optimization we should only dial once at the beginning and then keep using that connection on further pings

[AlbertoSoutullo]

Yes, if keep-alive is set to false then we wouldn't open those ping connections every 2 minutes that make the open connections to grow indefinitely

Then I will need to doublecheck this in our simulations, because we are not using that flag...