omnilib / aiosqlite

asyncio bridge to the standard sqlite3 module
https://aiosqlite.omnilib.dev
MIT License
1.2k stars 94 forks source link

make .close() method idempotent #238

Closed zzzeek closed 1 year ago

zzzeek commented 1 year ago

While not explicitly documented in pep-249, repeated calls to connection.close() are customarily accepted without error; the sqlite3 module itself allows multiple calls to .close() as do popular asyncio database drivers such as asyncpg.

Prior to this change, the .close() method would attempt to execute a command within the thread, however since the _connection attribute is None, it would raise ValueError("no active connection"). This ValueError is also not pep-249 compliant since all exceptions raised should be subclasses of dbapi.Error, so if the aiosqlite project would prefer repeated .close() calls raise, it should raise within the dbapi.Error hierarchy.

The change here is submitted by the maintainers of SQLAlchemy (https://www.sqlalchemy.org) for the benefit of projects to reduce warnings emitted during various connection, application, and interpreter shutdown scenarios.

following the Contributor guide I did not see specifics on how to add changelog entries and no issue number was required.

zzzeek commented 1 year ago

seems I missed the "make format".

that's pushed now

amyreese commented 1 year ago

Thank you!

abompard commented 1 year ago

Hey folks! I've been hit by this because a rollback() was automatically issued on the second close(), and the rollback() operation is not protected as the close() is by this PR. Do you think it would make sense to add the same protection to rollback()?