Explicitly close the opened connection

[ecederstrand]

If a connection pool is created in a multiprocess environment, e.g. in settings.py of a Django app hosted by uWSGI with multiple workers, spawning processes will inherit the open fd and may close a connection that the parent process is still using.

Unfortunately, I was unable to create a simple test case to trigger this.

[ecederstrand]

In view of issue #133, this was triggered with thriftpy 0.3.9 installed.

[wbolster]

hmmm i do not think this is the right approach.

this change makes the pool always close a connection when a connection is returned. this is contrary to the whole point of the pool, which is connection reuse. with this proposed change the pool would degrade to the equivalent of "open, use, close", which can also be achieved without using a pool in the first place,

that said, i think the right approach is to create the pool after forking, since connections are process-local.

also, a pool only makes sense for multithreaded (or green threaded) apps, multiprocessing does not do that so you could also make a connection directly. the only benefit of the pool would be connection recovery in case of failure.

[ecederstrand]

No, this does not always close a connection when it's returned. The patch only closes the initial connection that is created in the ConnectionPool constructor. ConnectionPool.connection() is left untouched.

I understand your argument, but it was surprising to me that merely creating a pool would leave an open connection. It would have been less surprising if I had actually used the pool.

We're running uWSGI in a multi-threaded, multi-process setting, so the connection pool still makes sense for us. I'm not at liberty to decide when uWSGI forks new processes.

[wbolster]

i think the pool should be opened after forking. isn't that the recommended approach for local resources (that do not survive a fork, like tcp connections) for multiprocessing environments?

[ecederstrand]

uWSGI somehow starts a worker process and forks it when it's ready to handle requests, and our connectionpool is created in the startup process. We want this for the same reasons that the connectionpool does the initial tasting of the connection, i.e. sanity checking at startup instead of on first use.

However, I see your point that this isn't a happybase problem per se, and I managed to fix the problem by using the close-on-exec uWSGI configuration setting instead.

[wbolster]

https://github.com/wbolster/happybase/blob/master/happybase/pool.py#L75-L79

perhaps a boolean flag to the pool makes more sense.

[luyun-aa]

Hi, we encountered the same problem here as well recently. I think the point is we expect the pool to be lazy, but making a connection upon construction doesn't seems so. I think adding a boolean flag to L75-L79 makes sense, at least the users have a way to disable the check. Would you accept such a patch?

[wbolster]

perhaps a autoconnect=True arg just like its Connection counterpart makes sense, with a proper docstring what it does.