Fix Engine.release method to release connection in any way #756
Conversation
Pliner
force-pushed
the
fix-332
branch
3 times, most recently
from
4ec2b5a to
65ab4da
Compare
Codecov Report
@@ Coverage Diff @@
## master #756 +/- ##
==========================================
- Coverage 92.77% 92.76% -0.02%
==========================================
Files 13 13
Lines 1565 1562 -3
Branches 180 179 -1
==========================================
- Hits 1452 1449 -3
Misses 81 81
Partials 32 32
Continue to review full report at Codecov.
|
Pliner
changed the title
| with pytest.raises( | ||
| (psycopg2.InterfaceError, psycopg2.OperationalError) | ||
| ): | ||
| with pytest.warns(ResourceWarning, match='Invalid transaction status'): |
There was a problem hiding this comment.
Is it ok to spam users with both warning and exception? Maybe it is better just to raise an exception? Connection reset is not a user's fault, so I think that this warning doesn't help them.
There was a problem hiding this comment.
Thanks for noticing this.
I'm a newbie in the aiopg codebase and don't know the reasons why these warnings are reported in such a way.
@asvetlov Could you share your opinion please?
There was a problem hiding this comment.
My initial thought was preventing a programming error and signal this situation early.
I agree that the connection releasing can be done for network reasons, there no need for extra checks.
I see two options:
- Drop exception in 'release' as PR does, https://github.com/aio-libs/aiopg/blob/master/aiopg/pool.py#L242-L248 warning also can be dropped. It can lead to reports about unfinished transactions by next acquired connection.
Satisfactory but confusing. - Raise an exception only if the underlying socket is not closed but swallow the transaction status for disconnected peers. It prevents false positives from network errors but can report a programming error fast. Connection for the unclosed transaction should be dropped immediately, the connection is in an undefined incorrect state and cannot be recovered. Perhaps a warning generated in the pool can be replaced with a strict error also.
The 1) is easier, 2) is harder to implement but maybe more correct.
I don't want to decide what to do; I have no time to contribute anyway. Please choose what you want.
There was a problem hiding this comment.
Okay, got you, thanks.
I've opened a new issue to fix it a bit later, because it looks like merging of the PR with incorrect warnings in quite rare case is much better than suffering from broken pool.
| with pytest.raises(sa.InvalidRequestError): | ||
| engine.release(conn) | ||
| with pytest.warns(ResourceWarning, match='Invalid transaction status'): | ||
| await engine.release(conn) |
There was a problem hiding this comment.
Maybe it is better to raise an exception here instead of a warning. Looks like a user's bug to release a connection in the middle of transaction.
There was a problem hiding this comment.
I've added this comment to the issue as well.
| while self.connections: | ||
| writer = self.connections.pop() | ||
| writer.close() | ||
| if hasattr(writer, "wait_closed"): |
There was a problem hiding this comment.
In python 3.6 there is no such a method
* upgrade aiopg to 1.2.1 fixes aio-libs/aiopg#756
The goal of the PR is to actualise the fix made in #558 by @vmagamedov, but with slightly reworked tests.
Fixes #332, #665, #666 as well.
It looks like both exceptions(
psycopg2.InterfaceErrorandpsycopg2.OperationalError) could be raised, see https://github.com/aio-libs/aiopg/pull/756/checks?check_run_id=1503163697, initially only the first one was expected in the tests.Tests are written using intermediate tcp proxy to allow disrupt a connection and cover two cases:
SELECT 1;for instance) is executed, so rollback could not be executed.