ensure connection is released

[thehesiod]

I've seen cases where the close can throw psycopg2.ProgrammingError: close cannot be used while an asynchronous query is underway which would previously cause the connection to not be returned to the pool, and cause a connection to "leak" from the pool. See discussion in: #364

This also fixes running tests on OSX

[jettify]

Good point, but should we also close connection in case we have any error on cursor.close(), this way pool will recycle it, I am not 100% sure that connection is usable after that exception.

[thehesiod]

@jettify but that's what the line that failed tried doing, closing it, and from what I saw on timeouts it tries to cancel the connection as well.

[thehesiod]

btw CI seems to be failing due to something I did not change:

    writing manifest file 'pip-egg-info/psycopg2.egg-info/SOURCES.txt'
    Error: could not determine PostgreSQL version from '10.1'

[jettify]

Regarding CI, I should have fix soon.

[jettify]

Failed line tried to do cursor.close()? but not con.close() or I am missing something?

[jettify]

Rebase should fix CI

[codecov]

Codecov Report

Merging #415 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #415   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files          25       25           
  Lines        3651     3651           
  Branches      194      194           
=======================================
  Hits         3415     3415           
  Misses        198      198           
  Partials       38       38

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7da9004...2cadd1d. Read the comment docs.

[thehesiod]

btw, let me know if you'd like to handle this via https://docs.python.org/3.5/library/contextlib.html#contextlib.ExitStack, I use it for situations like these to avoid code bloat

[jettify]

I like ExitStack too but change is not significant probably not worth it.

[jettify]

Regarding change, as you mentioned in other issue, we probably need to close connection, then return is back to the pool. Pool will drop it:

aiopg/aiopg/pool.py

Lines 250 to 261 in b301c9c

	if not conn.closed:
	tran_status = conn._conn.get_transaction_status()
	if tran_status != TRANSACTION_STATUS_IDLE:
	logger.warning(
	"Invalid transaction status on released connection: %d",
	tran_status)
	conn.close()
	return fut
	if self._closing:
	conn.close()
	else:
	self._free.append(conn)

[jettify]

I worry about returning potentially broken connection to the pool.

[thehesiod]

ok, so then what we need to do is create a new connection for the pool, let me c if I can come up with something

[jettify]

no need to create connection, pool should care about that
on each acquire we call following function just to see, may be we have free slots for new connections

aiopg/aiopg/pool.py

Lines 200 to 216 in b301c9c

	while self.size < self.minsize:
	self._acquiring += 1
	try:
	conn = yield from connect(
	self._dsn, loop=self._loop, timeout=self._timeout,
	enable_json=self._enable_json,
	enable_hstore=self._enable_hstore,
	enable_uuid=self._enable_uuid,
	echo=self._echo,
	**self._conn_kwargs)
	# raise exception if pool is closing
	self._free.append(conn)
	self._cond.notify()
	finally:
	self._acquiring -= 1
	if self._free:
	return

[thehesiod]

with minsize == maxsize == 1:

If we don't call release, and the connection never"closes" we're stuck because there are no "freeable" connections (default recycle is -1).

If we do call release (like this patch does), and the connection never closes, we append a potentially bad connection to the pool.

So I think we need "drop" ability to Pool.release

[jettify]

I mean close connection and release it back to the pool. Regardless of pool size connection should be always returned, it could be closed but should be returned.

[jettify]

drop ability is here:

aiopg/aiopg/pool.py

Lines 249 to 250 in b301c9c

	self._used.remove(conn)
	if not conn.closed:

we stop tracking connection and do not move it to free connection list

[thehesiod]

I'm not sure I'm following what you're saying but I've submitted a change which will allow non-closed connections to be dropped, let me know what you think.

[jettify]

Can you elaborate here? bit not follow in what case we need this?

[thehesiod]

i just realized the fundamental premise of this function is to reuse connections, so there's a problem when we're passing connections that are in a bad state (thus I reverted last change).

Here's the issue which we need to resolve: conn.close() consistently throws an exception, how do we drop it? Right now we're going to call release, which will add it back to the free pool. If we don't call release it will be stuck in the used pool.

[jettify]

Hold on but I thought that self._cur.close() throws exception in your case not conn.close() ?

[jettify]

I am thinking more about:

    def __exit__(self, *args):
        try:
            self._cur.close()
        except ProperException as e:
            self._conn.close()
        finally:
            try:
                self._pool.release(self._conn)
            finally:
                self._pool = None
                self._conn = None
                self._cur = None

[thehesiod]

ah, I was assuming that if the cursor close fails the connection close would fail as well, but ya I guess not, I'll code that up, thanks

[jettify]

Glad to see you find some time to code between your parental responsibilities :)

[thehesiod]

thanks, just sent you a link in gitter :)

[jettify]

not sure about this one, constructor also sets _obj property, it is not listed in slots, probably better have original version

[thehesiod]

actually, this seems more correct, _obj is used for __aenter__ / __aexit__ in the parent class, and this is overriding the behavior and thus should be using the same attribute

[thehesiod]

with even a further simplification :)

[jettify]

merged as part of #421