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

ConnectionPool::TimedStack is now lazy #52

Merged
merged 2 commits into from Mar 13, 2014

Conversation

Projects
None yet
3 participants
@drbrain
Copy link
Collaborator

drbrain commented Feb 18, 2014

Now TimedStack only creates connections when the internal queue is empty
and the number of connections is less than the maximum set. By default
no connections are created when the pool is created.

This required some changes to the TimedStack tests because I wrote poor
tests the first time around. The new tests better exercise the intended
behavior of TimedStack.

This required some changes to the ConnectionPool tests to use up
connections because now an unused pool has no connections.

ConnectionPool::TimedStack is now lazy
Now TimedStack only creates connections when the internal queue is empty
and the number of connections is less than the maximum set.  By default
no connections are created when the pool is created.

This required some changes to the TimedStack tests because I wrote poor
tests the first time around.  The new tests better exercise the intended
behavior of TimedStack.

This required some changes to the ConnectionPool tests to use up
connections because now an unused pool has no connections.
@mperham

This comment has been minimized.

Copy link
Owner

mperham commented Feb 18, 2014

Changing the semantics of the connection pool from eager to lazy is a very big semantic change and something worthy of a 2.0 version bump. I suspect many people depend on the behavior implicitly. I've not been a fan of lazy stuff when it comes to multi-threaded code as there's lots of room for race conditions. Perhaps I'm being too much of a crotchety old man.

@drbrain

This comment has been minimized.

Copy link
Collaborator

drbrain commented Feb 18, 2014

Future steps in this work are (in no particular order):

  • Allow creation of different "types" of connection (for Net::HTTP)
  • Add LRU for expiry of connections when full

The ultimate goal is to use connection_pool in net-http-persistent. connection_pool would handle handing out connections for whatever host the user needed to talk to while reaping connections that weren't recently used.

This could work with both a small number of connections (for connecting to an API) or a large number of connections (for web spider work) since connection_pool has accounting built-in.

When I'm done I think my changes will be 2.0-worthy. How would you best like to review my changes? The changes I've mentioned above will need to build atop each other which doesn't lend itself to pull requests particularly well.

@mperham

This comment has been minimized.

Copy link
Owner

mperham commented Feb 18, 2014

I still think PRs are the best approach, you can always target a staging branch, rather than master, so your changes can all be merged at once when the entire development process is complete.

@mperham

This comment has been minimized.

Copy link
Owner

mperham commented Mar 1, 2014

Would you bump the version to 2.0 and update the changelog?

@coelhone

This comment has been minimized.

Copy link

coelhone commented Mar 4, 2014

lazy loading? sounds familiar...

drbrain added a commit that referenced this pull request Mar 13, 2014

Merge pull request #52 from drbrain/lazy
ConnectionPool::TimedStack is now lazy

@drbrain drbrain merged commit 22ee821 into mperham:master Mar 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment