Issue: Fix threadlocal pool
Author: alexlatchford
State: closed
Created: 2016-09-16 01:17:51

So, I appreciate there are use cases for multiple connections open per thread but please don't shoot me down straight away!

Test code:

``` python
from peewee import *
from playhouse.pool import PooledPostgresqlExtDatabase

db = PooledPostgresqlExtDatabase(
    'my_app',
    user='postgres',
    threadlocals=True,
    register_hstore=False
)


def num_conn():
    return len(db._in_use)


print num_conn()
db.connect()
print num_conn()
db.connect()
print num_conn()
db.close()
print num_conn()
db.close()
print num_conn()
```

This outputs (against master):

``` bash
(env) alexlatchford@MacBook-Pro peewee $ python test.py 
0
1
2
1
1
```

Obviously results in an unexpected connection leak under normal user circumstances. I'd like to advocate for the introduction of the nesting flag so you can explicitly choose whether you desire this behaviour.

With this patch:

``` bash
(env) alexlatchford@MacBook-Pro peewee $ python test.py 
0
1
1
0
0
```

Any comments would be great, this is a bug that's taken me a while to track down and has been affecting us in production so would be great to get a solution in place! (I know ideally we just shouldn't open 2 connections nested, but with several devs, lots of code I'm trying to look for the pragamatic solution to wholly prevent this leaking behaviour)

Cheers,
Alex


Wait...isn't this the whole point of connection pooling?


Actually hold up, I see what you're talking about -- the issue isn't the pool, but the fact that peewee only holds a connection per thread.

If you need more than one connection (what is your use-case? I'm genuinely curious and not asking in an attempt to be patronizing), you can use the `ExecutionContext()`:

http://docs.peewee-orm.com/en/latest/peewee/database.html#advanced-connection-management

> Execution contexts allow finer-grained control over managing multiple connections to the database. When an execution context is initialized (either as a context manager or as a decorated function), a separate connection will be used for the duration of the wrapped block. You can also choose whether to wrap the block in a transaction.

There is already a mechanism in place for managing multiple conns per thread, so I will pass on this patch.

Let me know if this is unclear.


I think you've got the wrong end of the stick here. This patch fixes a connection leak which is unrecoverable, because you lose the reference unless you call `db.close_all()`.

When you open 2 connections in a nested fashion (which we do accidentally, our fault) I'd assumed that because it was outside of an `ExecutionContext` block it would just do a noop on the second `db.connect()` and reuse the same connection as it's on the same thread. Unfortunately that doesn't happen and instead it draws another connection from the pool, overwriting the reference to the first in the `Database` instance (but it's still referenced in `db._in_use` so it cannot be garbage collected or closed).

If you repeat this the number of times you've set your `max_connections` to then you've exhausted your pool and your app starts throwing `ValueError("Max connections exceeded")`.

Hope that makes some more sense.


This has always been the behavior, though I agree that an accident in your code can have disproportionately bad consequences. I'm reluctant to change it because the method is a verb, and implies an action taking place. If you want to have the "connect or re-use existing" behavior, that is what the `Database.get_conn()` method is for.

I can see an argument, however, for modifying the pool such that multiple calls to connect do not leak a connection. Reopening.


Cool I've switched over our code to use `get_conn()`, I hadn't come across that one yet and looks like it'll do the job for what we're after actually. I agree that muddling the actionability isn't ideal so happy to close this and hopefully this PR can serve as documentation for those in the same unlikely situation in the future if they go googling.

Just deployed this and looks like this solved our particular problem, thanks so much for your help!


Maybe the only change I'd suggest out of this would be to change the docs for the Pool to use an example with `get_conn` to highlight this problem.