Fix closing of pools

[adriangb]

I have a reproducible test where there are still connections left in pool._pool after close() is run. This seems to fix it for my case but I have no idea why and can't really share a reproducible example. My guess is it has something to do with a race condition and/or weirdness with pytest and event loops.

I'm opening this first to see what happens in CI.

[adriangb]

@dvarrazzo how would you feel about merging this given test pass? Does the change make sense to you at all?

[dvarrazzo]

This comment is now misplaced. "Now" refers to the code out of the self._lock block, not to _stop_workers(). (my bad, the lack of a blank line made it deceptive).

[adriangb]

Maybe it'd be best you propose a diff / push the change?

[dvarrazzo]

Ok, let me try and figure out a different codepath.

[dvarrazzo]

As you, I have no idea how you can get it the situation in which there are still connections in the pool. Maybe we call _stop_workers() in a moment in which a worker is busy creating a connection, and it will receive the StopWorker task only after it has added a new connection. Therefore I see how calling _pool.clear() afterwards can make a difference.

One thing that is note done right here is the _pool.clear() which doesn't close a connection eventually left there, which may result in a warning.

Looking at the _stop_worker(), I find the connections parameter extremely weird. It doesn't seem its responsibility to do that. Can you please try to refactor this code by removing the connections parameter from _stop_workers and then, as you did here, clear the pool, but closing the connection, after _stop_worker() call? Something like:

        with self._lock:
            # [snip]
            self._waiting.clear()

            self._stop_workers(waiting, timeout)

            for conn in self._pool:
                conn.close()
            self._pool.clear()

        # Now that the flag _closed is set, getconn will fail immediately,
        # putconn will just close the returned connection.

[dvarrazzo]

I see that _stop_workers() is also called by __del__(), but with no connection list, and also that it takes on itself the responsibility of closing the waiting clients... I don't know if this is really the best organisation of this code: feel free to propose cleanup refactoring if you see any obvious one.