-
Notifications
You must be signed in to change notification settings - Fork 162
Issue 120 add pool close #125
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -556,6 +556,17 @@ def run(): | |
t.start() | ||
t.join() | ||
|
||
def test_pool_close(): | ||
pool = ConnectionPool(size=3, **connection_kwargs) | ||
|
||
with pool.connection(): | ||
with assert_raises(TypeError): | ||
pool.close_connections(timeout='foo') | ||
|
||
with assert_raises(ValueError): | ||
pool.close_connections(timeout=0) | ||
|
||
pool.close_connections() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this actually does not test that the logic works. :) not sure about a sane test though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. me either :) ... I thought about mocking the connections, but that's a new pattern compared to the rest of the tests. as a result I wasn't sure it was a worthwhile path to go down. |
||
|
||
if __name__ == '__main__': | ||
import logging | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems to me that this destructively drains the pool by emptying the queue. the queue itself is an internal implementation detail so perhaps it should be changed into something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does. I don't have a good way to non-destructively drain the queue. the queue is an internal representation, but this code doesn't change that it only allows the caller to 'politely' terminate the connections when they see fit.
perhaps it's named badly? a better name might be "terminate_connections" the current name implies a different behavior.
thoughts? ... I feel like I'm missing your point a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, if you drain the queue, there won't be any connections to hand out anymore. that means the pool becomes useless after closing all the connections, which is bad :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It's intended as a terminal action... a final cleanup before you're done.