Skip to content
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.

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

Enable ConnectionPool to recover from zero capacity #128

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
56 changes: 40 additions & 16 deletions Sources/SwiftKuery/ConnectionPool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,23 @@ public class ConnectionPool {
// needed, so MUST later be returned to the pool with give() if it is to be reused.
// Items can therefore be borrowed or permanently removed with this method.
//
// This function will block until an item can be obtained from the pool.
// This function will block until an item can be obtained from the pool.
private func take() -> Connection? {
defer {
unlockPoolLock()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too early - you should only defer { unlockPoolLock() } after you have done a lockPoolLock(). Otherwise you may unlock when you weren't locked.


var item: Connection!

guard increasePoolSize() != nil else {
return nil
}

// If we took the last item, we can choose to grow the pool
if (pool.count == 0 && capacity < limit) {
_ = populateConnectionPool()
}

// Indicate that we are going to take an item from the pool. The semaphore will
// block if there are currently no items to take, until one is returned via give()
let result = semaphore.wait(timeout: (timeout == 0) ? .distantFuture : .now() + DispatchTimeInterval.milliseconds(timeout))
Expand All @@ -120,7 +134,6 @@ public class ConnectionPool {
// We have permission to take an item - do so in a thread-safe way
lockPoolLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

(it should go here)

if (pool.count < 1) {
unlockPoolLock()
return nil
}
item = pool[0]
Expand All @@ -129,21 +142,9 @@ public class ConnectionPool {
if item.isConnected == false {
releaser(item)
capacity -= 1
if let newItem = generator() {
capacity += 1
pool.append(newItem)
semaphore.signal()
}
}
// If we took the last item, we can choose to grow the pool
if (pool.count == 0 && capacity < limit) {
if let newItem = generator() {
capacity += 1
pool.append(newItem)
semaphore.signal()
}
return populateConnectionPool()
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't right - we should be returning an item that is not (currently) in the pool. It'll be returned to the pool later in give().

}
unlockPoolLock()

return item
}

Expand All @@ -166,6 +167,29 @@ public class ConnectionPool {
unlockPoolLock()
}

private func increasePoolSize() -> Connection? {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like name or return type of this function. This should be something like tryToSatisfyMinimumPoolSize() and should return a Bool indicating whether it was successful (ie. the pool is at the minimum size) or not.

defer {
unlockPoolLock()
}
lockPoolLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't really matter in this instance, but I would suggest flipping these round so that the defer is after the lock.

var connection: Connection? = nil
if capacity == 0 {
connection = populateConnectionPool()
}
return connection
}

private func populateConnectionPool() -> Connection? {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be adding a new item to the pool and returning it, it should do one or the other. It's not valid to take() a connection that is still in the pool, as that means two threads could get hold of it, and it would be give()n back twice.

if let newItem = generator() {
capacity += 1
pool.append(newItem)
semaphore.signal()
return newItem
} else {
return nil
}
}

private func lockPoolLock() {
_ = poolLock.wait(timeout: DispatchTime.distantFuture)
}
Expand Down