Skip to content
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

SSCursor can't close while raise Error #428

Closed
ppd0705 opened this issue Aug 15, 2019 · 17 comments · Fixed by #646 or #761
Closed

SSCursor can't close while raise Error #428

ppd0705 opened this issue Aug 15, 2019 · 17 comments · Fixed by #646 or #761
Labels
Milestone

Comments

@ppd0705
Copy link

ppd0705 commented Aug 15, 2019

if server send a error to SSCurosr, SSCursor try to call self._result._finish_unbuffered_query in close function

# SSCursor close funcion in  cursor.py line 604
if self._result is not None and self._result is conn._result:
    await self._result._finish_unbuffered_query()

but there is not a byte to read after recived a error packge, so the loop is allways waiting in the time, the error could't raise out .

# _finish_unbuffered_query function in connection.py line 1197  
while self.unbuffered_active:
    packet = await self.connection._read_packet()

I met the bug after I seted a mysql variable

SET GLOBAL MAX_EXECUTION_TIME=3000

a SELECT statement will be aborted if it takes more than 3s.

the connection received a error package with erorno 3024, but the loop is waiting packge to finitshed unbuffered_query

@ppd0705
Copy link
Author

ppd0705 commented Aug 16, 2019

Can someone respond to me?

@terrycain
Copy link
Collaborator

Can you post a reproducible test case

@ppd0705
Copy link
Author

ppd0705 commented Aug 17, 2019

you can reproduce this by 2 steps:

  1. set MAX_EXECUTION_TIME for the Mysql server
  2. select a large data that spend time more than the MAX_EXECUTION_TIME, use SScursor in WITH statement, simple code like
async with conn.cursor(SScursor) as cursor:
    await cursor.execute("SELECT a large data")
    await ret = cursor.fetchall()

the conn received a error package with errorno 3024 and msg "Query execution was interrupted, maximum statement execution time exceeded ".
before raise error, the cursor.close() is called because the WITH statement.
then coon is waiting a EOF package to finish unbuffed query, but mysql server do not need a to send EOF package in this satuation.

@ppd0705
Copy link
Author

ppd0705 commented Sep 15, 2019

I fixed the issue for pymysql in the commit

@KOR-Believer
Copy link
Contributor

Hello @ppd0705.
I also faced the same problem.

SELECT /*+ MAX_EXECUTION_TIME(3000) */ col1, col2 FROM table #some heavy query

If aiomysql updates the pymysql version to 0.10.0 or higher, will the problem be solved?

If the aiomysql does not update, is there any other alternative?

@ppd0705
Copy link
Author

ppd0705 commented Dec 21, 2021

this bug can only be fixed in aiomysql and has nothing to do with the version of pymysql.
maybe you can pull a request to fix it.

KOR-Believer added a commit to KOR-Believer/aiomysql that referenced this issue Jan 5, 2022
Fix to raise an error when query execution timeout in server-side cursor
- PyMySQL >=0.10.0,<=0.10.1 (to use MysqlPacket->raise_for_error())
@Nothing4You Nothing4You linked a pull request Jan 16, 2022 that will close this issue
4 tasks
@Nothing4You
Copy link
Collaborator

Nothing4You commented Feb 3, 2022

As I'm working on creating test cases for this I don't believe this is resolved yet, not resolved in PyMySQL either.

On aiomysql without the patch from #646 the connection gets stuck, which matches the original report of this issue by @ppd0705.

With #646 applied I'm seeing the same behavior as in PyMySQL, which I believe is still wrong:

The initial query succeeds, even though it should result in a timeout.
After this query, when issuing another query, I'm receiving the unrelated OperationalError 3024, which should have been raised for the previous execute().

reproducible test case moved to PyMySQL/PyMySQL#1032 (comment)

I wouldn't expect this to fail on select 1.

@KOR-Believer
Copy link
Contributor

KOR-Believer commented Feb 4, 2022

@Nothing4You
MAX_EXECUTION_TIME issues cannot be simulated with SLEEP().
You need an actual slow query.
That's why I couldn't write a test case.

@Nothing4You
Copy link
Collaborator

Nothing4You commented Feb 4, 2022

they actually can, you need to carefully craft the queries though.

example (mysql 8.0):

mysql> SELECT /*+ MAX_EXECUTION_TIME(1) */ data, sleep(1) FROM test;
ERROR 3024 (HY000): Query execution was interrupted, maximum statement execution time exceeded

from mysql 8.0 docs about SLEEP():

When SLEEP() is the only thing invoked by a query that is interrupted, it returns 1 and the query itself returns no error.

When SLEEP() is only part of a query that is interrupted, the query returns an error

the key takeaway is that sleep cannot be the only part of the query.

see for example https://github.com/Nothing4You/PyMySQL/blob/8c373feb9036b4c19eb338220f47586f26e5afd0/pymysql/tests/test_cursor.py#L137

@Nothing4You
Copy link
Collaborator

this is also similar to how MySQL implements this in their own tests: https://github.com/mysql/mysql-server/blob/mysql-8.0.28/mysql-test/t/max_statement_time.test

@Nothing4You
Copy link
Collaborator

definitely fun to test, in some cases I can even get PyMySQL to raise a ProgrammingError from SELECT /*+ MAX_EXECUTION_TIME(1) */ *,sleep(1) FROM tbl WHERE name='a', with a low chance though, i suspect related to whther the query is cached on MySQL side.

@Nothing4You
Copy link
Collaborator

continuing in PyMySQL/PyMySQL#1032

@Nothing4You
Copy link
Collaborator

tests for PyMySQL have been built, see PyMySQL/PyMySQL#1033

@Nothing4You
Copy link
Collaborator

Nothing4You commented Feb 6, 2022

(failing) tests can be seen in https://github.com/Nothing4You/aiomysql/actions/runs/1801465357
as I haven't bothered with implementing some sort of per test timeout yet, so this currently effectively only shows the process hanging at some point.

test cases are added in #728

Nothing4You added a commit that referenced this issue Feb 6, 2022
Nothing4You added a commit that referenced this issue Feb 6, 2022
fixes #428 (#646)

Co-authored-by: Richard Schwab <gitrichardschwab-7a2qxq42kj@central-intelligence.agency>
@Nothing4You
Copy link
Collaborator

reopening for the missing test case for #428 (comment)

@Nothing4You Nothing4You reopened this Feb 6, 2022
@Nothing4You
Copy link
Collaborator

Nothing4You commented Feb 6, 2022

as described in PyMySQL/PyMySQL#1032 (comment) onwards I don't think it's realistic to write a test case for this specific issue due to lack of determinism.

I believe porting PyMySQL/PyMySQL#1035 should be the final fix needed to resolve this.

@ppd0705 ppd0705 closed this as completed Feb 8, 2022
@Nothing4You Nothing4You reopened this Feb 8, 2022
@Nothing4You
Copy link
Collaborator

PyMySQL/PyMySQL#1035 isn't ported yet - once I've done that I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants