search

vacp2p / nim-libp2p

libp2p implementation in Nim
https://vacp2p.github.io/nim-libp2p/docs/
MIT License
240 stars 52 forks source link

chore: improve max outgoing connections log #1129

Closed gabrielmer closed 1 week ago

gabrielmer commented 1 week ago

Running into the following log

TRC 2024-06-17 15:33:42.400+00:00 Too many outgoing connections!             topics="libp2p connmanager" tid=7 file=connmanager.nim:344 count=0 max=50

The count parameter is a little bit misleading and at first makes one think that there's 0 connections. Looking at the code I see that it refers to the count field from AsyncSemaphore which indicates the available slots.

Changed the logging from count to available to avoid confusion, same as it is done in the semaphore module. For example https://github.com/vacp2p/nim-libp2p/blob/02f6e6127cecf5b87ff027780be42e9b692a0db9/libp2p/utils/semaphore.nim#L38

gabrielmer commented 1 week ago

LGTM! Thanks for it! 💯 Nevertheless, I wonder if we could also indicate the number of used/occupied connections

So I thought about it at first but didn't do it because of the existence of the following proc

https://github.com/vacp2p/nim-libp2p/blob/02f6e6127cecf5b87ff027780be42e9b692a0db9/libp2p/utils/semaphore.nim#L65-L70

Which means that the number of connections is not necessarily maximum - available and I didn't see in ConnManager a field with the actual number of connections

Ivansete-status commented 1 week ago

LGTM! Thanks for it! 💯 Nevertheless, I wonder if we could also indicate the number of used/occupied connections

So I thought about it at first but didn't do it because of the existence of the following proc

https://github.com/vacp2p/nim-libp2p/blob/02f6e6127cecf5b87ff027780be42e9b692a0db9/libp2p/utils/semaphore.nim#L65-L70

Which means that the number of connections is not necessarily maximum - used and I didn't see in ConnManager a field with the actual number of connections

ah okay, I was thinking that maybe we can use: c.outSema.queue.len

gabrielmer commented 1 week ago

ah okay, I was thinking that maybe we can use: c.outSema.queue.len

I understood that the queue is for connections waiting for an available slot, not existing connections. But let's see what @diegomrsantos says :))

diegomrsantos commented 1 week ago

@gabrielmer Thank you for improving this. Regarding your question, I don't know this part of the lib.