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 concurrency issues of parallel transactions (#327) #328

Merged
merged 12 commits into from Aug 17, 2021

Conversation

goteguru
Copy link
Contributor

This is the proposed solution for async transaction execution failure (see #327 )

Tests are running fine for postgres, mysql, sqlite.

I had to change the test tests/test_databases.py because it incorrectly expects database.transaction() to use the same connection as the connection context around it, which is certainly not true (and actually the part of the problem). If I acquire a connection, I have to use that connection for a transaction. (Requesting the database directly may give any other connection from the pool).

All comments are welcome.

@ToGoBananas
Copy link

looks good, no?

@erakli
Copy link

erakli commented Aug 6, 2021

@tomchristie take a look at this PR please

databases/core.py Outdated Show resolved Hide resolved
databases/core.py Outdated Show resolved Hide resolved
databases/core.py Outdated Show resolved Hide resolved
Co-authored-by: Egor Panfilov <erakli@users.noreply.github.com>
@tomchristie
Copy link
Member

@tomchristie take a look at this PR please

I'm happy to help out with expanding the maintainers team as needed, but I don't have any time to invest in databases myself.

@aminalaee
Copy link
Member

@goteguru Please run linting/formatting scripts in the scripts folder.

@goteguru
Copy link
Contributor Author

@tomchristie take a look at this PR please

I'm happy to help out with expanding the maintainers team as needed, but I don't have any time to invest in databases myself.

Thx for the honest reply. In the meantime I've replaced databases with direct asyncpg calls in my project (I don't like if local changes of dependencies unmerged to upstream), but I believe my future (concurrent) project can profit from this fix.

Maybe it would be nice to list the active project maintainers (w/ commit right) in the project page.

@goteguru
Copy link
Contributor Author

goteguru commented Aug 11, 2021

@aminalaee

@goteguru Please run linting/formatting scripts in the scripts folder.

I already did manually (all tests are passed), but I run them again for isort.
BTW: some windows commit corrupted the line endings, therefore the scripts will fail on linux using dash for sh (eg. all ubuntu).

Please consider recommended best practices while working from a windows machine:
git:formatting and whitespace

tests/test_databases.py Show resolved Hide resolved
Copy link
Member

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

Thank you @goteguru

@aminalaee aminalaee linked an issue Aug 17, 2021 that may be closed by this pull request
@aminalaee aminalaee merged commit cc3246a into encode:master Aug 17, 2021
sthagen added a commit to sthagen/encode-databases that referenced this pull request Aug 17, 2021
fix concurrency of parallel transactions (encode#328)
@gertvdijk
Copy link

gertvdijk commented Aug 26, 2021

Cool. Looks like this could fix #294, #230 and #176 too.

Although, I have switched away from using databases too, in favor of sqlalchemy + asyncpg.

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.

Async execution of transactions results in deadlock
6 participants