Fix race condition in Pool::acquire #1315
Conversation
| if !self.idle_conns.is_empty() { | ||
| return Poll::Ready(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this solution is going to trade one problem for another by making the pool unfair.
If there are connections in the idle queue, it allows a newly arriving task to go "ooh, actually I'll just take a connection and be on my way," even if there are other tasks waiting already.
Even if you check waiters.is_empty(), if the waiting tasks have already been signaled to wake but haven't taken a connection yet, they won't be in the queue any longer.
I think this only works if you check that the task is the head of the queue which isn't possible with the current implementation. In 0.6 I'm switching to an intrusive linked list for waiters which they can add themselves to as soon as they begin checking for a task.
Or alternatively we could replace most of this with a semaphore from futures-intrusive where acquiring a connection looks like this:
- acquire a permit from the semaphore
- try to pop a connection from the idle queue
- otherwise, open a new connection
Where anytime a connection is dropped or closed it releases a permit to the semaphore. This would also allow the pool to grow dynamically as we could just release additional permits to the semaphore at any time (although we'd need to change the idle queue to a SegQueue or else wrap it in a RwLock).
I previously shot down a recommendation to use futures-intrusive because I didn't care for the API but it seems to have improved to the point that we can use it for this.
The SharedSemaphore type uses an Arc internally though which is an unnecessary indirection in this case, IMO, but the regular semaphore type allows you to manually release permits so we can implement our own guard with that.
I have some time today so I think I will actually take a shot at that solution.
|
Closed in favor of #1320 |
Closes #1293