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

change run_sync #539

Merged
merged 5 commits into from
Jun 13, 2022
Merged

change run_sync #539

merged 5 commits into from
Jun 13, 2022

Conversation

dantownsend
Copy link
Member

Fixes #536

We recently changed the run_sync implementation to avoid a Python 3.10 deprecation warning. However, it had unintended consequences, and would sometimes fail when used alongside other async code.

This new implementation seems to avoid the deprecation warnings, whilst still working correctly with other async code.

@dantownsend dantownsend added the bug Something isn't working label Jun 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2022

Codecov Report

Merging #539 (8164881) into master (e1d5456) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #539   +/-   ##
=======================================
  Coverage   90.75%   90.76%           
=======================================
  Files         104      104           
  Lines        7011     7018    +7     
=======================================
+ Hits         6363     6370    +7     
  Misses        648      648           
Impacted Files Coverage Δ
piccolo/engine/postgres.py 87.55% <100.00%> (+0.55%) ⬆️
piccolo/utils/sync.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1d5456...8164881. Read the comment docs.

Trying `asyncio.run` first, as in 99% of cases this will work.

Also, using asyncio.run in a thread when running in an event loop.
This runs the tests in Python's development mode, which gives lots of useful information - for example, about deprecated features, and asyncio warnings.
When running the tests in development mode, I realised that there were situations where the connection wasn't being closed (i.e. if an exception was raised by asyncpg due to a Postgres error).
These are async first. They replace `create_tables` and `drop_tables` which were sync first, and had no async equivalent.
@dantownsend dantownsend added enhancement New feature or request and removed bug Something isn't working labels Jun 13, 2022
@dantownsend
Copy link
Member Author

In the end, I took another approach. It seems like asyncio.run is really the only dependable way to start an event loop in Python > 3.10, so that's what I'm using.

The root cause of this issue was using Piccolo's create_tables alongside another async library (ward). The problem is create_tables is sync only - it was a weird inconsistency in Piccolo's API. So now, it has been deprecated and replaced with:

  • create_db_tables - async first
  • create_db_tables_sync - a sync wrapper

Likewise, drop_tables has been replaced with an async first version:

  • drop_db_tables - async first
  • drop_db_tables_sync - a sync wrapper

@dantownsend dantownsend merged commit 99a77ae into master Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async ward tests fail with piccolo 0.75.0, pass with piccolo 0.74.4
2 participants