Skip to content
This repository was archived by the owner on Aug 19, 2025. It is now read-only.

Delay global_connection and global_transaction initialization to latest possible moment#158

Merged
gvbgduh merged 2 commits intoencode:masterfrom
rafalp:fix-157
Mar 16, 2020
Merged

Delay global_connection and global_transaction initialization to latest possible moment#158
gvbgduh merged 2 commits intoencode:masterfrom
rafalp:fix-157

Conversation

@rafalp
Copy link
Copy Markdown
Contributor

@rafalp rafalp commented Nov 3, 2019

This PR changes Database implementation so global connection is created only when its actually needed: when connection with backend is initialized. That way connection and its locks are initialized within same event loop, preventing issue documented in #157.

I've also had to change async_adapter implementation so it creates new event loop on every use instead of reusing already-existing event loop, which allowed me to reproduce error described in #157. It should also make tests run in env closer to "real" usage.

@rafalp rafalp force-pushed the fix-157 branch 7 times, most recently from 82a57c8 to 190b8e3 Compare November 3, 2019 20:16
@rafalp rafalp changed the title [WIP] Delay global_connection and global_transaction initialization to latest possible moment Delay global_connection and global_transaction initialization to latest possible moment Nov 3, 2019
@lovelydinosaur
Copy link
Copy Markdown
Contributor

Seems reasonable yup - what's the behavior of the test case without this change?

@rafalp
Copy link
Copy Markdown
Contributor Author

rafalp commented Mar 16, 2020

@tomchristie if you were to merge only tests part of this PR, then test case would reproduce my initial issue, and tests would fail with RuntimeError: Task <Task pending coro=<Database.execute() running at /usr/local/lib/python3.7/site-packages/databases/core.py:122> cb=[gather.<locals>._done_callback() at /usr/local/lib/python3.7/asyncio/tasks.py:691]> got Future <Future pending> attached to a different loop error.

@lovelydinosaur
Copy link
Copy Markdown
Contributor

Looks all v sensible to me.
I'll leave this for anyone else on the maintainance team to merge, then we should start looking at adding more PyPI permissions for releases.

@gvbgduh
Copy link
Copy Markdown
Contributor

gvbgduh commented Mar 16, 2020

Looks safe to merge anyway!

But for future we can revise the async_adapter itself and async clients, I'm under impresion it's a bit more convenient to run everything within one event loop (or like one loop per worker/process), which is generally provided by pytest.mark.asyncio in tests.
There was some confusion with the starlette's test_client that creates the conflicting event loop. Also some confusion in scenarious, running databases independently or within the service driven by starlette.

@gvbgduh gvbgduh merged commit 57197d7 into encode:master Mar 16, 2020
@rafalp rafalp deleted the fix-157 branch March 16, 2020 16:33
lovelydinosaur added a commit that referenced this pull request Apr 23, 2020
lovelydinosaur added a commit that referenced this pull request Apr 23, 2020
This reverts commit c18b19c.
lovelydinosaur added a commit that referenced this pull request Apr 23, 2020
* Failing test case for database.transaction() as a decorator

* Rollback #158

* Tweak .force_rollback

* Revert "Tweak .force_rollback"

This reverts commit 2835db7.

* Revert "Rollback #158"

This reverts commit c18b19c.

* Fix transction decorator

* Linting

* Version 0.3.1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants