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

Add asyncmy support #7000

Closed
wants to merge 4 commits into from
Closed

Add asyncmy support #7000

wants to merge 4 commits into from

Conversation

long2ice
Copy link
Contributor

@long2ice long2ice commented Sep 8, 2021

Description

Add asyncmy support, another asyncio driver for mysql

See #6993

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

Have a nice day!

@synodriver
Copy link

Wonderful!

@zzzeek
Copy link
Member

zzzeek commented Sep 13, 2021

can we have this set up in tox.ini / setup.cfg so that tests can be run against it? did you run the tests?

@zzzeek
Copy link
Member

zzzeek commented Sep 13, 2021

can i please have commit access to your branch?

@long2ice
Copy link
Contributor Author

sure

@zzzeek
Copy link
Member

zzzeek commented Sep 14, 2021

it's still rejecting me. can you please apply this patch to your branch, we can then pull into jenkins and have the tests run

commit dbd94944faad707dc78fa1628b74782ae6810454
Author: Mike Bayer <mike_mp@zzzcomputing.com>
Date:   Mon Sep 13 13:37:17 2021 -0400

    set up for tox
    
    Change-Id: I7c6702e5998fba49be72d763c1be52edc60f2687

diff --git a/setup.cfg b/setup.cfg
index d88cb13d11..a55822286a 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -74,6 +74,9 @@ pymysql =
 aiomysql =
     %(asyncio)s
     aiomysql;python_version>="3"
+asyncmy =
+    %(asyncio)s
+    asyncmy;python_version>="3"
 aiosqlite =
     %(asyncio)s
     aiosqlite;python_version>="3"
@@ -164,6 +167,8 @@ mysql = mysql://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4
 pymysql = mysql+pymysql://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4
 aiomysql = mysql+aiomysql://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4
 aiomysql_fallback = mysql+aiomysql://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4&async_fallback=true
+asyncmy = mysql+asyncmy://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4
+asyncmy_fallback = mysql+asyncmy://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4&async_fallback=true
 mariadb = mariadb://scott:tiger@127.0.0.1:3306/test
 mssql = mssql+pyodbc://scott:tiger^5HHH@mssql2017:1433/test?driver=ODBC+Driver+13+for+SQL+Server
 mssql_pymssql = mssql+pymssql://scott:tiger@ms_2008
diff --git a/tox.ini b/tox.ini
index 3cf11ba2e7..42750f5d11 100644
--- a/tox.ini
+++ b/tox.ini
@@ -31,6 +31,7 @@ deps=
      mysql: .[mysql]
      mysql: .[pymysql]
      mysql: git+https://github.com/sqlalchemy/aiomysql@sqlalchemy_tox; python_version >= '3'
+     mysql: .[asyncmy]; python_version >= '3'
      mysql: .[mariadb_connector]; python_version >= '3'
 
      oracle: .[oracle]
@@ -102,9 +103,9 @@ setenv=
     py2{,7}-mysql: MYSQL={env:TOX_MYSQL_PY2K:{env:TOX_MYSQL:--db mysql}}
     mysql: EXTRA_MYSQL_DRIVERS={env:EXTRA_MYSQL_DRIVERS:--dbdriver mysqldb --dbdriver pymysql}
 
-    py3{,5,6,7,8,9}-mysql: EXTRA_MYSQL_DRIVERS={env:EXTRA_MYSQL_DRIVERS:--dbdriver mysqldb --dbdriver pymysql --dbdriver mariadbconnector --dbdriver aiomysql}
+    py3{,5,6,7,8,9}-mysql: EXTRA_MYSQL_DRIVERS={env:EXTRA_MYSQL_DRIVERS:--dbdriver mysqldb --dbdriver pymysql --dbdriver mariadbconnector --dbdriver aiomysql --dbdriver asyncmy}
     # omit aiomysql for Python 3.10
-    py3{,10,11}-mysql: EXTRA_MYSQL_DRIVERS={env:EXTRA_MYSQL_DRIVERS:--dbdriver mysqldb --dbdriver pymysql --dbdriver mariadbconnector}
+    py3{,10,11}-mysql: EXTRA_MYSQL_DRIVERS={env:EXTRA_MYSQL_DRIVERS:--dbdriver mysqldb --dbdriver pymysql --dbdriver mariadbconnector --dbdriver asyncmy}
 
 
     mssql: MSSQL={env:TOX_MSSQL:--db mssql}

@long2ice
Copy link
Contributor Author

I apply it

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 398064b of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 398064b: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3079

@zzzeek
Copy link
Member

zzzeek commented Sep 14, 2021

thank you

@CaselIT
Copy link
Member

CaselIT commented Sep 14, 2021

btw you need to check this
image
in the pr page

@long2ice
Copy link
Contributor Author

That's it already
image

@zzzeek
Copy link
Member

zzzeek commented Sep 15, 2021

pushing is not working, but we are just about done

$ git push asyncmy HEAD
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 8 threads
Compressing objects: 100% (7/7), done.
Writing objects: 100% (8/8), 729 bytes | 729.00 KiB/s, done.
Total 8 (delta 6), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (6/6), completed with 6 local objects.
To github.com:long2ice/sqlalchemy.git
 ! [remote rejected]       HEAD -> asyncmy (permission denied)
error: failed to push some refs to 'github.com:long2ice/sqlalchemy.git'

apply this patch please, should fix the rest

diff --git a/lib/sqlalchemy/dialects/mysql/asyncmy.py b/lib/sqlalchemy/dialects/mysql/asyncmy.py
index 657298cc7c..3dac325af6 100644
--- a/lib/sqlalchemy/dialects/mysql/asyncmy.py
+++ b/lib/sqlalchemy/dialects/mysql/asyncmy.py
@@ -161,6 +161,7 @@ class AsyncAdapt_asyncmy_ss_cursor(AsyncAdapt_asyncmy_cursor):
 
     def close(self):
         if self._cursor is not None:
+            self.await_(self._cursor.fetchall())
             self.await_(self._cursor.close())
             self._cursor = None
 
diff --git a/lib/sqlalchemy/dialects/mysql/base.py b/lib/sqlalchemy/dialects/mysql/base.py
index 9bf12e194c..c9c77c925c 100644
--- a/lib/sqlalchemy/dialects/mysql/base.py
+++ b/lib/sqlalchemy/dialects/mysql/base.py
@@ -2915,7 +2915,7 @@ class MySQLDialect(default.DefaultDialect):
                 "WHERE TABLE_TYPE='SEQUENCE' and TABLE_NAME=:name AND "
                 "TABLE_SCHEMA=:schema_name"
             ),
-            dict(name=sequence_name, schema_name=schema),
+            dict(name=util.text_type(sequence_name), schema_name=util.text_type(schema)),
         )
         return cursor.first() is not None
 

@long2ice
Copy link
Contributor Author

done

@CaselIT CaselIT requested review from sqla-tester and removed request for sqla-tester September 16, 2021 05:40
@zzzeek
Copy link
Member

zzzeek commented Sep 16, 2021

Is the bot not picking these up? I'll have to push manually

@zzzeek zzzeek requested review from sqla-tester and removed request for sqla-tester September 16, 2021 14:52
@zzzeek
Copy link
Member

zzzeek commented Sep 16, 2021

i hope github didnt change the stupid API

@zzzeek
Copy link
Member

zzzeek commented Sep 16, 2021

permissions issue of some kind

Exception: Got response 403 for https://api.github.com/repos/sqlalchemy/sqlalchemy/collaborators/zzzeek/permission: b'{"message":"Must have push access to view collaborator permission.","documentation_url":"https://docs.github.com/rest/reference/repos#get-repository-permissions-for-a-user"}'

@zzzeek zzzeek removed the request for review from sqla-tester September 16, 2021 15:08
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision f7d6c81 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset f7d6c81 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3079

@zzzeek
Copy link
Member

zzzeek commented Sep 17, 2021

oh hey, this is your DBAPI , great! I can get it all working but there is two areas that the driver doesnt handle well.

One is that the connection raises AttributeError if you try to call it after it's closed, this should be InterfaceError:

import asyncio

import asyncmy

async def main():
    asyncmy_connection = await asyncmy.connect(user="scott", password="tiger", host="localhost", db="test")

    # fine
    await asyncmy_connection.ping(False)

    asyncmy_connection.close()

    # attribute error
    await asyncmy_connection.ping(False)

asyncio.run(main())

output:

$ python test3.py 
Traceback (most recent call last):
  File "/home/classic/dev/sqlalchemy/test3.py", line 16, in <module>
    asyncio.run(main())
  File "/usr/lib64/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/home/classic/dev/sqlalchemy/test3.py", line 14, in main
    await asyncmy_connection.ping(False)
  File "asyncmy/connection.pyx", line 494, in ping
  File "asyncmy/connection.pyx", line 487, in asyncmy.connection.Connection.ping
  File "asyncmy/connection.pyx", line 686, in _execute_command
  File "asyncmy/connection.pyx", line 625, in asyncmy.connection.Connection._write_bytes
AttributeError: 'NoneType' object has no attribute 'write'

the other is that if you have an unconsumed server side cursor, you can't close the cursor or do anything like roll back the connection until the cursor is fully consumed, it should clean this up automatically:

import asyncio

import asyncmy

async def main():
    asyncmy_connection = await asyncmy.connect(user="scott", password="tiger", host="localhost", db="test")

    ss_cursor = asyncmy_connection.cursor(asyncmy.cursors.SSCursor)

    await ss_cursor.execute("select 1")

    # uncomment to resolve
    #await ss_cursor.fetchall()

    # fails
    await asyncmy_connection.rollback()

asyncio.run(main())

output:

$ python test4.py 
/home/classic/dev/sqlalchemy/test4.py:16: UserWarning: Previous unbuffered result was left incomplete
  await asyncmy_connection.rollback()
/home/classic/dev/sqlalchemy/test4.py:16: RuntimeWarning: coroutine 'MySQLResult._finish_unbuffered_query' was never awaited
  await asyncmy_connection.rollback()
Traceback (most recent call last):
  File "/home/classic/dev/sqlalchemy/test4.py", line 18, in <module>
    asyncio.run(main())
  File "/usr/lib64/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/home/classic/dev/sqlalchemy/test4.py", line 16, in main
    await asyncmy_connection.rollback()
  File "asyncmy/connection.pyx", line 374, in rollback
  File "asyncmy/connection.pyx", line 337, in _read_ok_packet
  File "asyncmy/connection.pyx", line 591, in read_packet
asyncmy.errors.InternalError: Packet sequence number wrong - got 4 expected 1
sys:1: RuntimeWarning: coroutine 'MySQLResult._finish_unbuffered_query' was never awaited

the AttributeError is more important, I've worked around it for now. thanks for the contribution! I'm finishing this up now and obviously asyncmy moves to the top of the list of MySQL asyncio drivers.

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3079 has been merged. Congratulations! :)

@long2ice
Copy link
Contributor Author

oh hey, this is your DBAPI , great! I can get it all working but there is two areas that the driver doesnt handle well.

One is that the connection raises AttributeError if you try to call it after it's closed, this should be InterfaceError:

import asyncio

import asyncmy

async def main():
    asyncmy_connection = await asyncmy.connect(user="scott", password="tiger", host="localhost", db="test")

    # fine
    await asyncmy_connection.ping(False)

    asyncmy_connection.close()

    # attribute error
    await asyncmy_connection.ping(False)

asyncio.run(main())

output:

$ python test3.py 
Traceback (most recent call last):
  File "/home/classic/dev/sqlalchemy/test3.py", line 16, in <module>
    asyncio.run(main())
  File "/usr/lib64/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/home/classic/dev/sqlalchemy/test3.py", line 14, in main
    await asyncmy_connection.ping(False)
  File "asyncmy/connection.pyx", line 494, in ping
  File "asyncmy/connection.pyx", line 487, in asyncmy.connection.Connection.ping
  File "asyncmy/connection.pyx", line 686, in _execute_command
  File "asyncmy/connection.pyx", line 625, in asyncmy.connection.Connection._write_bytes
AttributeError: 'NoneType' object has no attribute 'write'

the other is that if you have an unconsumed server side cursor, you can't close the cursor or do anything like roll back the connection until the cursor is fully consumed, it should clean this up automatically:

import asyncio

import asyncmy

async def main():
    asyncmy_connection = await asyncmy.connect(user="scott", password="tiger", host="localhost", db="test")

    ss_cursor = asyncmy_connection.cursor(asyncmy.cursors.SSCursor)

    await ss_cursor.execute("select 1")

    # uncomment to resolve
    #await ss_cursor.fetchall()

    # fails
    await asyncmy_connection.rollback()

asyncio.run(main())

output:

$ python test4.py 
/home/classic/dev/sqlalchemy/test4.py:16: UserWarning: Previous unbuffered result was left incomplete
  await asyncmy_connection.rollback()
/home/classic/dev/sqlalchemy/test4.py:16: RuntimeWarning: coroutine 'MySQLResult._finish_unbuffered_query' was never awaited
  await asyncmy_connection.rollback()
Traceback (most recent call last):
  File "/home/classic/dev/sqlalchemy/test4.py", line 18, in <module>
    asyncio.run(main())
  File "/usr/lib64/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/home/classic/dev/sqlalchemy/test4.py", line 16, in main
    await asyncmy_connection.rollback()
  File "asyncmy/connection.pyx", line 374, in rollback
  File "asyncmy/connection.pyx", line 337, in _read_ok_packet
  File "asyncmy/connection.pyx", line 591, in read_packet
asyncmy.errors.InternalError: Packet sequence number wrong - got 4 expected 1
sys:1: RuntimeWarning: coroutine 'MySQLResult._finish_unbuffered_query' was never awaited

the AttributeError is more important, I've worked around it for now. thanks for the contribution! I'm finishing this up now and obviously asyncmy moves to the top of the list of MySQL asyncio drivers.

Fixed that already and wait release

@CaselIT
Copy link
Member

CaselIT commented Sep 18, 2021

If you could post here what version will fix there we may decide to set that one as the lowest supported.

thanks for the contribution!

@long2ice
Copy link
Contributor Author

Of course, https://github.com/long2ice/asyncmy/releases/tag/v0.2.0

@CaselIT
Copy link
Member

CaselIT commented Sep 18, 2021

@zzzeek do you think it would makes sense to set 0.2.0 as the lowest supported version?

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

Patch Set 3:

CaselIT@github wrote:

@zzzeek do you think it would makes sense to set 0.2.0 as the lowest supported version?

looks like it's released, i dont think it makes that much difference but i want to do another gerrit run in any case lets see if i can remove my workarounds

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3079

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

looks like i can remove my cursor workarounds with 0.2.0 so i can gerrit that. however if it has a new error code for "this connection is closed" we'd need to code for that too when it comes out, separate issue (right now it raises AttributeError).

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3079

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

Successfully merging this pull request may close these issues.

None yet

5 participants