Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
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
27 changes: 27 additions & 0 deletions happybase/pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 is not None:
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))
Copy link
Member

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?

Copy link
Author

@smferguson smferguson May 24, 2016

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.

Copy link
Member

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 :(

Copy link
Author

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.


@contextlib.contextmanager
def connection(self, timeout=None):
"""
Expand Down
11 changes: 11 additions & 0 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

@smferguson smferguson May 20, 2016

Choose a reason for hiding this comment

The 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
Expand Down