Force dial #696
Force dial #696
Conversation
| @@ -77,13 +78,14 @@ proc release*(s: AsyncSemaphore) = | |||
| trace "Releasing slot", available = s.count, | |||
| queue = s.queue.len | |||
|
|
|||
| s.count.inc | |||
There was a problem hiding this comment.
this doesn't guarantee that count goes above 0 - should it really complete queued futures then?
There was a problem hiding this comment.
That's how the incoming doesn't get starved: the slot "created" by a forceAcquire can be reused for incoming connections.
Of course, after a while this slot will be deleted (because more stuff will be released than acquired)
There was a problem hiding this comment.
ok - hm, that is a little bit more permissive than I imagined it - it's a good point about starvation though, it does give more balance in return for longer periods of "non-compliance" - let's document this in code in case it causes trouble we didn't anticipate
| if s.queue.len > 0: | ||
| var fut = s.queue[0] | ||
| s.queue.delete(0) | ||
| if not fut.finished(): | ||
| fut.complete() | ||
| s.count.dec |
There was a problem hiding this comment.
as general hygiene, it's probably better to mutate the state before "releasing" the work that depends on the state update to have happened, even if de-facto, the work that gets released happens later
There was a problem hiding this comment.
Yeah, this proc is not async, but better be clean
| @@ -77,13 +78,14 @@ proc release*(s: AsyncSemaphore) = | |||
| trace "Releasing slot", available = s.count, | |||
| queue = s.queue.len | |||
|
|
|||
| s.count.inc | |||
| if s.queue.len > 0: | |||
| var fut = s.queue[0] | |||
| s.queue.delete(0) | |||
| if not fut.finished(): | |||
There was a problem hiding this comment.
this code looks odd btw: why would there be finished futures in the queue?
There was a problem hiding this comment.
and as a follow-up, if a future is finished already, should the "next" future be completed instead?
There was a problem hiding this comment.
A future could be cancelled
Not sure why we don't try another, I switched to a loop and every tests pass, @dryajov?
There was a problem hiding this comment.
the cancelled futures remove themselves from the queue in their cancellation callback, so that should never happen..
what could happen is that the user "manually" completes the future but that's not how one uses futures in general so..
There was a problem hiding this comment.
Right, missed that part
There was a problem hiding this comment.
if we can't figure out a reason, might be a good idea to assert with a message that the future was unexpectedly completed - by extension, a loop would no longer be needed - if this happens, I get the feeling we might get an imbalance in acquires and releases which either leads to lockups or overallocation
There was a problem hiding this comment.
Since the counter and the queue size can now diverge due to the forceAcquire contraption, we might as well have to use a loop in release.
This PR allows applications to dial peers even if we reached the connection limit
A new semaphore slot will be temporarily created, so the connection can never fail because of peer limitation
Alternative for #687, needed for status-im/nimbus-eth2#3346