-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix MySQL 8.0 tests, properly close timed out connections #660
Conversation
Codecov Report
@@ Coverage Diff @@
## master #660 +/- ##
==========================================
+ Coverage 83.52% 85.70% +2.17%
==========================================
Files 12 12
Lines 2052 2085 +33
Branches 331 336 +5
==========================================
+ Hits 1714 1787 +73
+ Misses 268 228 -40
Partials 70 70
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
45b55e6
to
381f006
Compare
15d6edf
to
112c11c
Compare
for issue documentation sake, these are the values on a timed out mysql 8.0 connection returned from the pool without this:
unix socket:
|
And the actual error packet returned on MySQL 8.0: |
Hi there! Just chipping in to express my interest in this PR being merged/released :) |
ensure connections are closed properly when the server connection was lost
…bute `_eof` on asyncio.StreamReader
3073d9c
to
61f7dce
Compare
What do these changes do?
Currently tests on timed out connections fail on MySQL 8.0.
This ports PyMySQL/PyMySQL#540.
Interestingly, the PyMySQL issue originally referred to MariaDB, however, I don't see this behavior on currently supported MariaDB versions.
MySQL 8.0 does behave this way though.
In addition to porting the connection related changes, due to us having a pool, we will also need to adjust the pool logic.
Currently the connection pool attempts to not return closed connections in
Pool.acquire
viaPool._fill_free_pool
.At this time the connection is not yet considered closed, that will only be determined on the next
Connection._read_packet
.When MySQL 8.0 closes the connection on us e.g. due to a timeout, the connection will still have the packet containing the error message as a pending read. The TCP connection stays in
CLOSE_WAIT
state for this.Having received EOF on the connection it isn't useful to us anymore, so while we may have data pending to be read we don't care about it.
There shouldn't be a case where we expect data available to be read here.
Unfortunately I was not able to find a public API that would provide this information on the connection opened by
asyncio.open_connection()
, using internal attributes it is possible to readStreamReader._eof
.To avoid accessing this internal attribute I've added a custom
StreamReader
that will expose whetherreader.feed_eof()
was called, which thePool
can then rely on.This is logically exposing the same information that would otherwise be available as
StreamReader._eof
.I was not yet able to properly this against unix socket connections.
On minimal local testing using the 2 previously failing test cases the same behavior shows on unix sockets and is also resolved with the same custom
StreamReader
.#664 should be implemented before the next release though to ensure we don't have any other unexpected issues on unix sockets.
After doing my own port I also realized that we already have a pending PR to port this (#358) since end of 2018.
While #358 also ports the same PyMySQL patch it does not account for closed connections in the pool, especially as the closed connection state is only discovered the next time the connection is accessed.
That may however be due to the issue being reported (in #340) for MariaDB 10.2 back then and the tests only covering MariaDB up to 10.1.
It may still be worth to migrate and merge the test cases from #358 though.
Are there changes in behavior for the user?
Users accessing timed out MySQL 8.0 connections may previously have received
pymysql.err.InternalError
instead of anpymysql.err.OperationalError
, this is now fixed.Users retrieving MySQL 8.0 connections from a pool will no longer receive timed out connections.
Connections are now properly closed before raising a
pymysql.err.OperationalError
due to losing connection to the server.Connections are now properly closed before raising a
pymysql.err.InternalError
when packet sequence numbers are out of sync.Related issue number
May fix #340, may fix #614
Checklist
CHANGES.txt