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

Fix connection leak when transaction commit/rollback raise an exception #498

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

masipcat
Copy link

@masipcat masipcat commented Jul 18, 2022

This fixes the following case:

from databases import Database

import asyncio


async def method():
    database = Database("postgresql://...")
    await database.connect()

    try:
        async with database.connection() as connection:
            await connection.execute(
                """
                CREATE TABLE test (
                    id integer PRIMARY KEY INITIALLY DEFERRED
                );
                """
            )
            async with connection.transaction():
                await connection.execute("insert into test (id) values (1)")
                await connection.execute("insert into test (id) values (1)")
                # The exception UniqueViolationError is raised during txn.commit()
    finally:
        # Without the fix, connection._connection_counter is 1
        print("Before disconnect:", connection._connection_counter)
        # This blocks forever waiting for the leaked connection
        await database.disconnect()
        print("After disconnect")


asyncio.run(method())

@aminalaee
Copy link
Member

Hi, thanks for the PR.
Can you maybe include the tests for it?

@masipcat
Copy link
Author

Yeah no problem I'll add a test

Comment on lines +554 to +570
async with Database(database_url) as database:
with pytest.raises(Exception) as excinfo:
async with database.connection() as connection:
await connection.execute(
"""
CREATE TABLE test (id integer PRIMARY KEY INITIALLY DEFERRED);
"""
)
async with connection.transaction():
await connection.execute("insert into test (id) values (1)")
await connection.execute("insert into test (id) values (1)")

# During transaction.commit() postgres will raise this exception:
# asyncpg.exceptions.UniqueViolationError: duplicate key value violates unique constraint "test_pkey"
# DETAIL: Key (id)=(1) already exists.
assert "unique constraint" in str(excinfo.value)
assert connection._connection_counter == 0
Copy link
Author

Choose a reason for hiding this comment

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

@aminalaee I think this test it's only valid for postgresql. Is there a way to skip for other databases?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sure,

async def test_transaction_commit_serializable(database_url):

tests/test_databases.py Outdated Show resolved Hide resolved
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.

2 participants