Fix connection pooling behavior when maxsize > 1

[berg]

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. :)

[shazow]

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?

[piotr-dobrogost]

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? :)

[berg]

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. :)

[piotr-dobrogost]

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.

[shazow]

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.

[berg]

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

[piotr-dobrogost]

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.

[shazow]

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

[shazow]

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

[piotr-dobrogost]

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

[shazow]

@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?

[piotr-dobrogost]

Sure.