Fixes Engine.release method to release connection in any way

[vmagamedov]

Fixes #332 error.

The problem is that Engine.release method raises InvalidRequestError exception before releasing connection. This may happen not only when user did something wrong, but also when connection becomes broken. Pool becomes exhausted and application stops responding.

I think that it doesn't matter who is guilty, connection should be released in any way, and Pool.release also checks connection's state, and if it is not in IDLE state, pool closes connection.

I've added test_release_broken_connection to reproduce this error. Currently Pool.release emits ResourceWarning, I think that it is enough.

[codecov]

Codecov Report

Merging #558 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #558      +/-   ##
==========================================
+ Coverage   94.35%   94.36%   +0.01%     
==========================================
  Files          27       27              
  Lines        3740     3747       +7     
  Branches      171      170       -1     
==========================================
+ Hits         3529     3536       +7     
  Misses        179      179              
  Partials       32       32

Impacted Files	Coverage Δ
aiopg/sa/engine.py	100% <ø> (ø)	⬆️
tests/test_sa_engine.py	99.02% <100%> (+0.1%)	⬆️

---

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 9fdf7b9...8084eb2. Read the comment docs.

[popravich]

@vir-mir , can you, please, take a look?

[vir-mir]

this behavior is fixed in PR #548

[vmagamedov]

This PR isn't directly corresponds to the #332, so maybe there is a misunderstanding. In #548 Engine.release still raises InvalidRequestError before releasing connection to the pool, so there is a possibility of pool exhaustion. Will try to reproduce this error in resolve_issues_535_and_364 branch on Monday to confirm that this PR is still actual.

[vmagamedov]

@vir-mir I checked out resolve_issues_535_and_364 branch and in my test case Engine.release wasn't called at all.

I see that you added _EngineAcquireContextManager and rely there sorely on SAConnection.close() coroutine. But take a closer look into it:

aiopg/aiopg/sa/connection.py

Lines 337 to 348 in 0c98538

	if self._connection is None:
	return
	if self._transaction is not None:
	await self._transaction.rollback()
	self._transaction = None
	# don't close underlying connection, it can be reused by pool
	# conn.close()
	self._close_weak_list_cursor()
	self._engine.release(self)
	self._connection = None
	self._engine = None

If await self._transaction.rollback() fails, self._engine.release(self) isn't called, connection isn't returned to the pool, pool gets exhausted.

You can reproduce this with my test_release_broken_connection. And I've successfully reproduced pool exhaustion by limiting maxsize to 1 and trying to acquire a new connection in the end of my test.

[vmagamedov]

BTW, without proper connection release engine.wait_closed() waits indefinitely, so make_engine fixture will hung on teardown of my test.

[vir-mir]

thank. I will try to solve it soon.

[oleksandr-kuzmenko]

@vir-mir hello! Did you have time for this PR?

[brianmaissy]

+1 for this issue

[Pliner]

+1 for the issue. @vir-mir Could you review PR, please?

[Pliner]

The same fix was applied in #756 and released as 1.1.0b1