Enable ConnectionPool to recover from zero capacity #128
Conversation
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
==========================================
- Coverage 85.25% 84.85% -0.41%
==========================================
Files 43 43
Lines 3575 3592 +17
==========================================
Hits 3048 3048
- Misses 527 544 +17
Continue to review full report at Codecov.
|
| @@ -166,6 +171,24 @@ public class ConnectionPool { | |||
| unlockPoolLock() | |||
| } | |||
|
|
|||
| private func isConnectionPoolPopulated() -> Bool { | |||
There was a problem hiding this comment.
This function conflates two things: "is the connection pool populated?", a boolean test of the current pool status, and "attempt to expand the pool if it's empty".
It's not clear from the function name that it can potentially modify state in this way.
I think the logic should be reworked.
There was a problem hiding this comment.
I think the objective of the function is okay (to check to see if the pool is at zero capacity, and attempt to recover from this if necessary) but the naming needs improvement. Something like tryToEnsureCapacityIsNonZero().
The problem is that capacity being zero indicates some problem has occurred (network down, DB down) - that all connections have been purged from the pool, as the generator was unable to replace them - and so we can reasonably expect the generator to fail here too. At which point, we need to exit take() early so as not to block.
However, once the DB is reachable again, this function can kick-start the process of growing the pool back up (only possible once there is capacity > 0, as the 'grow the pool' code is within the semaphore.wait()).
One other possibility that I discussed with @ddunn2 is to move the 'grow the pool' logic to the top of the take() function, and using it to also seed the pool with the first item if it has become dead.
| private func take() -> Connection? { | ||
| defer { | ||
| unlockPoolLock() | ||
| } |
There was a problem hiding this comment.
This is too early - you should only defer { unlockPoolLock() } after you have done a lockPoolLock(). Otherwise you may unlock when you weren't locked.
| @@ -120,7 +134,6 @@ public class ConnectionPool { | |||
| // We have permission to take an item - do so in a thread-safe way | |||
| lockPoolLock() | |||
| pool.append(newItem) | ||
| semaphore.signal() | ||
| } | ||
| return populateConnectionPool() |
There was a problem hiding this comment.
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().
| defer { | ||
| unlockPoolLock() | ||
| } | ||
| lockPoolLock() |
There was a problem hiding this comment.
It doesn't really matter in this instance, but I would suggest flipping these round so that the defer is after the lock.
| @@ -166,6 +167,29 @@ public class ConnectionPool { | |||
| unlockPoolLock() | |||
| } | |||
|
|
|||
| private func increasePoolSize() -> Connection? { | |||
There was a problem hiding this comment.
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.
| return connection | ||
| } | ||
|
|
||
| private func populateConnectionPool() -> Connection? { |
There was a problem hiding this comment.
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.
|
Closing this as updated fix can be found here |
Adds a check in the
take()method calledisConnectionPoolPopulated()which checks the current capacity. If the capacity is zero it attempts to create a new connection and add it to the pool.Failing this
nilis returned.