feat: add autorestore_context option and make it True by default#20
feat: add autorestore_context option and make it True by default#20spumer wants to merge 3 commits into
Conversation
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
Reviewer's Guide by SourceryThis pull request introduces an Sequence diagram for sync_to_async with autorestore_contextsequenceDiagram
participant SyncFn as Synchronous Function
participant geventGreenlet as gevent.Greenlet
participant greenlet_to_future as greenlet_to_future
participant asyncioEventLoop as asyncio Event Loop
participant Future as asyncio.Future
SyncFn->>geventGreenlet: gevent.Greenlet(fn, *args, **kwargs)
activate geventGreenlet
geventGreenlet->>greenlet_to_future: greenlet_to_future(greenlet, autorestore_context=True)
activate greenlet_to_future
greenlet_to_future->>asyncioEventLoop: loop.run_in_executor(fn)
activate asyncioEventLoop
asyncioEventLoop-->>greenlet_to_future: Result from fn
deactivate asyncioEventLoop
greenlet_to_future->>Future: Resolves with result
deactivate greenlet_to_future
Future-->>geventGreenlet: Result
geventGreenlet-->>SyncFn: Result
deactivate geventGreenlet
opt autorestore_context is True
geventGreenlet->>SyncFn: Restore context variables
end
Sequence diagram for async_to_sync with autorestore_contextsequenceDiagram
participant AsyncCoroutine as Asynchronous Coroutine
participant future_to_greenlet as future_to_greenlet
participant geventGreenlet as gevent.Greenlet
participant asyncioEventLoop as asyncio Event Loop
participant Future as asyncio.Future
AsyncCoroutine->>future_to_greenlet: future_to_greenlet(coroutine(), autorestore_context=True)
activate future_to_greenlet
future_to_greenlet->>asyncioEventLoop: loop.create_task(coroutine())
activate asyncioEventLoop
asyncioEventLoop-->>future_to_greenlet: Future
deactivate asyncioEventLoop
future_to_greenlet->>geventGreenlet: gevent.Greenlet(run_until_complete, future)
activate geventGreenlet
geventGreenlet->>Future: Await Future
Future-->>geventGreenlet: Result
deactivate geventGreenlet
future_to_greenlet-->>AsyncCoroutine: Result
deactivate future_to_greenlet
opt autorestore_context is True
geventGreenlet->>AsyncCoroutine: Restore context variables
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @spumer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a helper function to avoid repeating the context restoration logic in
async_to_syncandgreenlet_to_future. - The added tests are great for demonstrating the context preservation, but consider adding tests that verify context isolation between greenlets.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@gfmio can you review too?) |
There was a problem hiding this comment.
Pull Request Overview
This PR adds an autorestore_context option (enabled by default) to preserve context variables when converting between sync and async functions, making async_to_sync, sync_to_async, and related internals context‐aware and transparent.
- Introduce
autorestore_contextparameter insync_to_asyncandasync_to_syncand propagate it togreenlet_to_future - Capture, pass, and restore
contextvarsin_await_greenletandfuture_to_greenlet - Add tests to verify context preservation across greenlet/future boundaries
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_asyncio_on_gevent.py | Add tests for context preservation in sync_to_async and async_to_sync |
| asyncio_gevent/sync_to_async.py | Added autorestore_context parameter and forwarded it to greenlet_to_future |
| asyncio_gevent/greenlet_to_future.py | Capture contextvars before start, restore after await, updated signature and executor |
| asyncio_gevent/future_to_greenlet.py | Propagate gr_context when scheduling coroutine futures |
| asyncio_gevent/async_to_sync.py | Added autorestore_context parameter and restore context after sync call |
Comments suppressed due to low confidence (3)
asyncio_gevent/greenlet_to_future.py:82
- The return type of
greenlet_to_futurewas changed fromasyncio.Futuretotyping.Awaitable, which may be a breaking API change. Consider documenting this change or providing backward compatibility.
) -> typing.Awaitable:
tests/test_asyncio_on_gevent.py:97
- [nitpick] Test function name is verbose and uses multiple underscores; consider renaming to
test_sync_to_async_context_preservationfor clarity.
def test_asyncio_on_gevent___sync_to_async__pass_and_copyback_context():
tests/test_asyncio_on_gevent.py:1
- The tests reference
asyncio_geventbut there is noimport asyncio_geventstatement. Addimport asyncio_geventat the top to avoid NameError.
import contextvars
|
Thank you for using I've just had a quick look and I think I tried to merge in Can you please address those and do the deduplication the bot reviewers have highlighted? Once that's taken care of, I'll merge in your PR and cut version 0.3.0. Thanks! |
gfmio
left a comment
There was a problem hiding this comment.
Can you please address the two issues? The potential deadlock issue is a definite blocker as it stands.
The main reason for this behavior is we make usage of async_to_sync and sync_to_async is transparent.
Programmers can use it without any _unexpected_ side effects.
… parameter Remove global ThreadPoolExecutor(max_workers=1) in greenlet_to_future that caused deadlocks with concurrent calls. Add optional executor parameter (default None = asyncio default executor). Extract _ensure_future helper in future_to_greenlet to deduplicate coroutine scheduling logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
31df0cf to
a8cc526
Compare
|
@gfmio deadlock tests implemented and i found a way to exclude ThreadPoolExecutor from implementation.
This allow switch between EvenLoop greenlet and otherone, like between threads. So, detailed overview in module docstring. This allow us use "await future" without locking forever. You can replace |
|
Before that call_soon_threadsafe was called in ThreadPoolExecutor (greenlet.join). When a spawned greenlet completes, TWO events occur in parallel: Path 1 (link callback): Path 2 (executor): UPD: good article about asyncio event loop and self-pipe mechanics: https://habr.com/ru/articles/995032/ |
|
Can we run tests or review via AI? |
The main reason for this behavior is we make usage of async_to_sync and sync_to_async is transparent.
Programmers can use it without any unexpected side effects.
So, i can wait for your PR #19 and adapt my changes.
We use your lib on production since 2023 and i wrote article about that:
https://habr.com/ru/companies/tochka/articles/798577/
We run sync handlers in fastapi through asyncio-gevent, and this save us! Thank you for this project!
Summary by Sourcery
Add context preservation functionality to async and sync function conversions in asyncio-gevent
New Features:
autorestore_contextoption that preserves context variables when converting between sync and async functionsEnhancements:
async_to_sync,sync_to_async, andgreenlet_to_futurefunctions to support context variable preservationTests:
sync_to_asyncandasync_to_syncfunction conversions