miguelgrinberg / greenletio

Asyncio integration with sync code using greenlets.
MIT License
150 stars 8 forks source link

Improve portability #10

Closed spumer closed 1 year ago

spumer commented 1 year ago

Hi! I suggest few changes. If you accept it i will write tests for each feat.

I was try to replace fastapi "run_in_threadpool" by "run_in_greenletio". I'm failed with ThreadPoolExecutors which used in my tests, cause they don't patched correctly. And this changes i was made due implement this "feature" in our project.

Our stack is fastapi + django + psycopg2. And we write sync handlers and async handler only for huge-load API methods. In few places (fastapi.Depends) we use asgiref.sync_to_async, this is why i add options to async_ decorator.

Patching loop.run_until_complete is from fastapi test infrastructure and their sync TestClient.

Changelog:

- feat: now GreenletBridge allow use loop.run_until_complete to run async code from sync

loop.run_untilcomplete - is common "gateway" from sync to async world. Now it's just replaced by `await` which is "common" for greenletio

codecov-commenter commented 1 year ago

Codecov Report

Merging #10 (56c69b7) into main (66d57e5) will decrease coverage by 2.54%. The diff coverage is 57.14%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##              main      #10      +/-   ##
===========================================
- Coverage   100.00%   97.46%   -2.54%     
===========================================
  Files            8        8              
  Lines          458      474      +16     
  Branches        43       49       +6     
===========================================
+ Hits           458      462       +4     
- Misses           0        9       +9     
- Partials         0        3       +3     
Impacted Files Coverage Δ
src/greenletio/core.py 84.00% <55.55%> (-16.00%) :arrow_down:
src/greenletio/green/threading.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

miguelgrinberg commented 1 year ago

I don't commit big PRs that include several unrelated features. To simplify your changes, I have separately committed an update to the build system to add python 3.10, 3.11 and pypy-3.8, and I have also added a fix for the iscoroutinefunction() problem you reported.

spumer commented 1 year ago

Ok, i opened it for discuss. What about copy_context feature? I did not found correct workaround to implement it outside, cause only wrapper know about greenlet and only wrapper can pass context

miguelgrinberg commented 1 year ago

@spumer please test the main branch. I have committed a more complete implementation of your patch that also works when exceptions are raised. I have also merged the two booleans that you used into one, with_context=True. I'm not sure there is a need to separate the sending and receiving of the context, having one option seems sufficient to me.

spumer commented 1 year ago

I was think about it, and split into two variable to allow follow two different behaviours:

asyncio.Task inherit context, but do not reflect changes back. coroutine share same context and this is why contextvars reflection needed.

But, may be it's over engineering. And we can just use asyncio.create_task(async_(fn)) to "protect" context from reflection.

spumer commented 1 year ago

please test the main branch.

All works fine!