Fix connection pooling behavior when maxsize > 1 #13

Merged
merged 1 commit into from Jan 29, 2012

Conversation

Projects
None yet
3 participants
Contributor

berg commented Nov 29, 2011

The connection pool's underlying Queue.Queue (a FIFO queue) is prefilled
with None objects; therefore, until you make maxsize HTTP requests, no
connections will be reused. I've attached a test which demonstrates this
(and now passes after switching to Queue.LifoQueue.)

The requests library defaults to a pool size of 10, which exposes this
bug.

Thanks for considering my patch! I didn't add myself to CONTRIBUTORS as this is a pretty minor fix. :)

@berg berg Fix connection pooling behavior when maxsize > 1
The connection pool's underlying Queue.Queue (a FIFO queue) is prefilled
with None objects; therefore, until you make maxsize HTTP requests, no
connections will be reused. I've attached a test which demonstrates this
(and now passes after switching to Queue.LifoQueue.)

The requests library defaults to a pool size of 10, which exposes this
bug.
5de8335
Owner

shazow commented Nov 29, 2011

Hi @failberg, you rock! Thank you for taking initiative on this.

One concern: LifoQueue is only introduced in Py26, but urllib3 strives to support Py25 (perhaps I should add that note to the README).

I would argue that this is technically not a bug, since when you specify "use up to 5 connections", urllib3 uses up to 5 connections. ;) But I do agree that connection reusing "early and often" is better.

I see a few options:

  1. Put this on ice until we drop Py25 support (or make a Py3 fork).
  2. Find a way to make LifoQueue backwards compat with Py25.
  3. Document how to extend ConnectionPool to use LifoQueue instead of a regular queue for those who care.

What do you think?

Contributor

piotr-dobrogost commented Nov 29, 2011

Progressive enhancement: use lifo in 2.6 and above and fifo otherwise. The simplest and makes the most people happy. Could we wish for more? :)

Contributor

berg commented Nov 29, 2011

Thanks for the quick reply!

You're absolutely right that this isn't technically a bug. I should've checked on the availability of LifoQueue in Py25--sorry about that. Since this is just a matter of making connection reuse "better," would you consider @piotr-dobrogost's suggestion of progressive enhancement? If you're opposed to that, it probably makes sense to take punt until Py25 support isn't a requirement.

I briefly investigated option #2 above and didn't find a drop-in compatible LifoQueue for 2.5 that would work and option #3 isn't ideal for my own selfish needs as I mostly consume urllib3 via requests--and it's unlikely that patch would be integrated there. :)

Contributor

piotr-dobrogost commented Nov 29, 2011

Also I would name the test case differently as bigpool doesn't say what is being tested. My proposal test_connection_count_connection_reuse or just ``test_connection_count_reuse`. In case of using different types of queues the test case needs to be modified accordingly.

Owner

shazow commented Nov 29, 2011

I am very weary of having different behavior transparently depending on what version of Python you're using. This is a usability/support/maintenance nightmare.

Contributor

berg commented Nov 29, 2011

Fair enough. In that case, let's put this on hold until Python 2.5 support is no longer a requirement.

Contributor

piotr-dobrogost commented Dec 13, 2011

FIFO is timeout friendly and LIFO is not so FIFO/LIFO behavior of pools should be configurable I guess.
This problem brings us to more general subject of managing pool behavior. There is unbound number of behaviors and supporting them all is only feasible by providing hooks so that users could define any behavior they want. This would allow load balancing as well.

Owner

shazow commented Dec 13, 2011

Hooks such as extending the ConnectionPool class and overwriting whatever
behaviour you like? :)

Owner

shazow commented Jan 23, 2012

Looks like we're dropping Py25 support very soon in light of #35, then we can merge this in. :) Keep an eye out.

Contributor

piotr-dobrogost commented Jan 23, 2012

@shazow What do you think about configuration option selecting FIFO or LIFO pool?

Owner

shazow commented Jan 23, 2012

@piotr-dobrogost I'm thinking perhaps defining the PoolQueueClass within the ConnectionPool class definition so that you can override it easily by extending or monkeypatching the respective class. Would that be good enough?

Contributor

piotr-dobrogost commented Jan 23, 2012

Sure.

@shazow shazow merged commit 5de8335 into shazow:master Jan 29, 2012

Owner

shazow commented on 5de8335 Jul 4, 2014

@berg TIL you contributed to urllib3! :D Thank you. :)

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