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

Enable ConnectionPool to recover from zero capacity #128

wants to merge 1 commit into from

Conversation

DunnCoding
Copy link

@DunnCoding DunnCoding commented Apr 16, 2018

Adds a check in the take() method called isConnectionPoolPopulated() 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 nil is returned.

@DunnCoding DunnCoding requested a review from djones6 April 16, 2018 13:30
@codecov-io
Copy link

codecov-io commented Apr 16, 2018

Codecov Report

Merging #128 into master will decrease coverage by 0.4%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#SwiftKuery 84.85% <0%> (-0.41%) ⬇️
Impacted Files Coverage Δ
Sources/SwiftKuery/ConnectionPool.swift 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a16fd5e...4c4e013. Read the comment docs.

@@ -166,6 +171,24 @@ public class ConnectionPool {
unlockPoolLock()
}

private func isConnectionPoolPopulated() -> Bool {
Copy link
Contributor

@ianpartridge ianpartridge Apr 16, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

This needs a rethink, there are several problems with the current proposal.

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.

@@ -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)

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().

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.

@@ -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.

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.

@DunnCoding
Copy link
Author

Closing this as updated fix can be found here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants