-
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 2 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 |
---|---|---|
|
@@ -90,6 +90,33 @@ def _return_connection(self, connection): | |
"""Return a connection to the pool.""" | ||
self._queue.put(connection) | ||
|
||
def close_connections(self, timeout=None): | ||
""" | ||
Attempts to politely close all connections in the pool. | ||
Waits for used connections to become available or until the | ||
timeout is exceeded. | ||
|
||
:param int timeout: number of seconds to wait for a connection to | ||
become available (optional) | ||
:return: None | ||
""" | ||
if timeout: | ||
if not isinstance(timeout, int): | ||
raise TypeError("close_connections 'timeout' arg must be an integer") | ||
|
||
if not timeout > 0: | ||
raise ValueError("close_connections 'timeout' arg must be greater than zero") | ||
|
||
while not self._queue.empty(): | ||
try: | ||
conn = self._queue.get(block=True, timeout=timeout) | ||
conn.close() | ||
conn = None | ||
except Queue.Empty: | ||
raise NoConnectionsAvailable( | ||
"Closing connections failed: No connection available from " | ||
"pool within specified timeout of {}".format(timeout)) | ||
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. 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 commentThe 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. 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. 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 commentThe 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. |
||
|
||
@contextlib.contextmanager | ||
def connection(self, timeout=None): | ||
""" | ||
|
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=-1) | ||
|
||
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 | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
should be
if timeout is not None: