feat: add AsyncSemaphore

[vladopajic]

No description provided.

[cheatfate]

I'm sorry, but could you please explain why your Semaphore implementation requires forceAcquire*(s: AsyncSemaphore) and count*(s: AsyncSemaphore)?

[vladopajic]

I'm sorry, but could you please explain why your Semaphore implementation requires forceAcquire*(s: AsyncSemaphore) and count*(s: AsyncSemaphore)?

this is semaphore from libp2p. this pr is adding it here so that it could be reused. it was implemented way before i joined the project so detailed reasoning i am not able to answer.

forceAcquire is used here: https://github.com/vacp2p/nim-libp2p/blob/master/libp2p/connmanager.nim#L343
count proc is read only for for available slots, used in same file few lines below.

[cheatfate]

I'm sorry, but could you please explain why your Semaphore implementation requires forceAcquire*(s: AsyncSemaphore) and count*(s: AsyncSemaphore)?

this is semaphore from libp2p. this pr is adding it here so that it could be reused. it was implemented way before i joined the project so detailed reasoning i am not able to answer.

forceAcquire is used here: https://github.com/vacp2p/nim-libp2p/blob/master/libp2p/connmanager.nim#L343 count proc is read only for for available slots, used in same file few lines below.

Semaphore from libp2p was not merged into chronos because of such things, this is definitely some kind of hacks being used by libp2p which are not generic. If you check semaphore primitive common api, you will never find such type of hacks.

Also as you can see here present alternative AsyncSemaphore implementation #147.

As a compromise i would propose to make some fields in AsyncSemaphore become public, so if libp2p still wants to use forceAcquire and/or count it could implement it inside libp2p repository.

[vladopajic]

As a compromise i would propose to make some fields in AsyncSemaphore become public, so if libp2p still wants to use forceAcquire and/or count it could implement it inside libp2p repository.

👍

[arnetheduck]

use case

the use case is the following: a semaphore is used to control whether tcp accept should be called or not, ie whether to process incoming connections - the same semaphore is used for outgoing connections as well, so that when the total amount is >50 (for example), we stop accepting new incoming connections but continue allowing outgoing until a higher limit is reached - force is used to allow a higher number of outgoing connections in case there is an imbalance, ie if we have many incoming connections, we allow the total connections to go up to 60 for example using force - accept will then not be called until we're below 50 again - it's a legitimate use case, ie we can think of it as a "priority lane" for some semaphore acquirers and "force" gives the correct semantic in this particular case.

without forceacquire one would have to adjust the upper limit and then down again which seems like the more complex solution.

[vladopajic]

i have removed forceAcquire and count from this implementation, mainly to follow @cheatfate suggestion.
as he suggested, we can extend semaphore in libp2p with these two methods and have traditional implementation here in chronos.

still from my perspective, there isn't anything hacky with forceAcquire and count (is only inappropriately named). and to me, exposing internals is more hackier then having nontraditional procs. if those procs are correctly implemented, tested and documented and there is no added cost to traditional implementation i don't see anything stopping us from having those.

please let me know how do we want to proceed with this.

[cheatfate]

I have checked Windows, pthreads, Python, C# and Rust implementation of Semaphore primitive, and did not find any proofs that forceAcquire is needed. I could agree that if count would be renamed to something like availableSlots or availablePermits it could get its life, but forceAcquire eliminates core feature of Semaphore primitive, which should not allow passing through number of slots/permits. So proposed specific version of Semaphore should not be named as AsyncSemaphore because it breaks Semaphore core feature.

So if you want your own specific version of Semaphore, please keep it in nim-libp2p. Because as generic version i would prefer to have something similar to #147.

[vladopajic]

@arnetheduck @cheatfate is there a concensus on this?

[arnetheduck]

forceAcquire

it just occured to me that libp2p can simply discard sem.acquire() and this will have the same effect ;)

[vladopajic]

forceAcquire

it just occured to me that libp2p can simply discard sem.acquire() and this will have the same effect ;)

haha that's smart actually. or even force dials can just avoid semaphore altogether?

[arnetheduck]

or even force dials can just avoid semaphore altogether?

consider that you have a limit of 6 connections and you want to maintain a balance of incoming and outgoing connections - in this case, if you have 6 connections already but they're all incoming, you want to start making outgoing connections without allowing incoming connections - iff some of those outgoing connections are successful, you can drop the "least useful" incoming connection and bring back balance, but you only know that at the end of the "outgoing attempt".

In the above example, you can make as many parallel outgoing attempts as you like, but for the sake of simplicity, let's assume 1.

While you're "attempting", you should continue to not allow incoming connections - ie if you start a connection attempt and 1 connection drops, you still don't want to allow more incoming connections - however, if 2 connections drop, you can allow one incoming (since the incoming + the outgoing attempt won't exceed 6 together).

This last bit is why we count all connections but "force" outgoing connections: because we have other mechanisms to control how many outgoing connections we make, while the limit is on outgoing+incoming and the incoming connections keep coming without us controlling when and how many.