Add lock to ReAwaitable for concurrent awaits #2109
+99
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
sobolevn
reviewed
Apr 11, 2025
|
|
||
|
|
||
| async def sample_coro(): | ||
| await anyio.sleep(0.1) |
There was a problem hiding this comment.
Suggested change
| await anyio.sleep(0.1) | |
| await anyio.sleep(1) |
give less chance for random failures.
|
|
||
| @pytest.mark.anyio | ||
| async def test_concurrent_awaitable(): | ||
| reawaitable = ReAwaitable(sample_coro()) |
There was a problem hiding this comment.
Please, add a link to the original bug on github.
| from returns.primitives.reawaitable import ReAwaitable | ||
|
|
||
|
|
||
| async def sample_coro(): |
There was a problem hiding this comment.
Suggested change
| async def sample_coro(): | |
| async def sample_coro() -> str: |
|
|
||
|
|
||
| @pytest.mark.anyio | ||
| async def test_concurrent_awaitable(): |
There was a problem hiding this comment.
Suggested change
| async def test_concurrent_awaitable(): | |
| async def test_concurrent_awaitable() -> None: |
returns/primitives/reawaitable.py
Outdated
| @@ -2,6 +2,8 @@ | |||
| from functools import wraps | |||
| from typing import NewType, ParamSpec, TypeVar, cast, final | |||
|
|
|||
| import anyio | |||
There was a problem hiding this comment.
It is not in list of production deps:
Lines 51 to 57 in bbfc3d3
Can we use ifs to find the correct lock type?
There was a problem hiding this comment.
I see, but unfortunately i am not familiar with other libraries than pure asyncio.
Is it appropriate to use asyncio.Lock?
There was a problem hiding this comment.
Thank you for the fast review!
There was a problem hiding this comment.
ah maybe something like this?
try:
import anyio
Lock = anyio.Lock
except ImportError:
import asyncio
Lock = asyncio.Lock
proboscis
force-pushed
the
add-reawaitable-lock
branch
from
0c8f6b6 to
4d646e9
Compare
proboscis
force-pushed
the
add-reawaitable-lock
branch
from
ec52eca to
94d5b1f
Compare
…ot available and improve test types
proboscis
force-pushed
the
add-reawaitable-lock
branch
from
33f89f9 to
3e7fed1
Compare
proboscis
force-pushed
the
add-reawaitable-lock
branch
from
c4300c0 to
2d8ae80
Compare
proboscis
force-pushed
the
add-reawaitable-lock
branch
from
1569389 to
a1206af
Compare
sobolevn
reviewed
Apr 11, 2025
tests/test_primitives/test_reawaitable/test_reawaitable_concurrency.py
Outdated
Show resolved
Hide resolved
tests/test_primitives/test_reawaitable/test_reawaitable_concurrency.py
Outdated
Show resolved
Hide resolved
tests/test_primitives/test_reawaitable/test_reawaitable_concurrency.py
Outdated
Show resolved
Hide resolved
…rency.py Co-authored-by: sobolevn <[email protected]>
…rency.py Co-authored-by: sobolevn <[email protected]>
Co-authored-by: sobolevn <[email protected]>
…rency.py Co-authored-by: sobolevn <[email protected]>
|
Thank you! I hope this works. |
sobolevn
reviewed
Apr 12, 2025
returns/primitives/reawaitable.py
Outdated
|
|
||
| Lock = asyncio.Lock | ||
| else: | ||
| Lock = anyio.Lock |
There was a problem hiding this comment.
You can type it as a simple interface that only supports async with, probably like contextlib.AbstractAsyncContextManager
sobolevn
requested changes
Apr 12, 2025
- Add notes that anyio is required for proper trio support - Add type annotation for Lock variable - Document the asyncio.Lock fallback behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
sobolevn
reviewed
Apr 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have made things!
Fix for issue: #2108
Checklist
Related issues
This PR addresses the issue where multiple concurrent awaits on the same ReAwaitable instance could lead to unexpected behavior. By adding a lock around the critical section in the _awaitable method, we ensure that the coroutine is only awaited once even when multiple tasks await the same ReAwaitable concurrently.
The PR includes a test case that demonstrates concurrent awaiting works correctly with the lock in place.
🙏 Please, if you or your company finds \ valuable, help us sustain the project by sponsoring it transparently on https://github.com/sponsors/dry-python.