Skip to content

Fix race condition in Pool::acquire #1315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix race condition in Pool::acquire #1315

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions sqlx-core/src/pool/inner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,10 @@ impl<DB: Database> SharedPool<DB> {
future::poll_fn(|cx| -> Poll<()> {
let waiter = waiter.get_or_insert_with(|| Waiter::push_new(cx, &self.waiters));

if !self.idle_conns.is_empty() {
return Poll::Ready(());
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #1320

if waiter.is_woken() {
waiter.actually_woke = true;
Poll::Ready(())
Expand Down