fix race condition - library can become not ready before the event.wait() returns#740
fix race condition - library can become not ready before the event.wait() returns#740
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
asab/library/service.py (1)
189-190: Add exception chaining to preserve the original timeout error in tracebacks.The current code raises a different exception type without chaining, which obscures the original
asyncio.TimeoutErrorin debugging context. Useraise ... from errto maintain exception causality:- except asyncio.TimeoutError: - raise LibraryNotReadyError("Library is not ready yet.") + except asyncio.TimeoutError as err: + raise LibraryNotReadyError("Library is not ready yet.") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asab/library/service.py` around lines 189 - 190, Catch the original asyncio.TimeoutError as a variable and re-raise LibraryNotReadyError using exception chaining so the original timeout is preserved in tracebacks: modify the except block handling asyncio.TimeoutError to use "except asyncio.TimeoutError as err" and then "raise LibraryNotReadyError(... ) from err" referencing the asyncio.TimeoutError and LibraryNotReadyError symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@asab/library/service.py`:
- Around line 189-190: Catch the original asyncio.TimeoutError as a variable and
re-raise LibraryNotReadyError using exception chaining so the original timeout
is preserved in tracebacks: modify the except block handling
asyncio.TimeoutError to use "except asyncio.TimeoutError as err" and then "raise
LibraryNotReadyError(... ) from err" referencing the asyncio.TimeoutError and
LibraryNotReadyError symbols.
| """ | ||
| if timeout is None: | ||
| timeout = self.LibraryReadyTimeout | ||
| if self.is_ready(): |
There was a problem hiding this comment.
Fast path is good. It avoids waiting when readiness already holds at entry itself.
| await asyncio.wait_for(self.LibraryReadyEvent.wait(), timeout=timeout) | ||
| except asyncio.TimeoutError: | ||
| raise LibraryNotReadyError("Library is not ready yet.") | ||
| self._ensure_ready() |
There was a problem hiding this comment.
This still feels like a one-time wait. We wake on the first ready event and then decide immediately from that one wake-up.
If the goal is still “wait until ready or timeout”, I think we probably need to keep waiting when that wake turns out to be stale, instead of treating it as the final truth for readiness.
There was a problem hiding this comment.
I would probably do something like this instead, so we keep waiting until self.is_ready() is actually true, while still respecting the original timeout.
async def wait_for_library_ready(self, timeout: int = None):
if timeout is None:
timeout = self.LibraryReadyTimeout
loop = asyncio.get_running_loop()
deadline = loop.time() + timeout
while True:
if self.is_ready():
return
remaining = deadline - loop.time()
if remaining <= 0:
raise LibraryNotReadyError("Library is not ready yet.")
try:
await asyncio.wait_for(self.LibraryReadyEvent.wait(), timeout=remaining)
except asyncio.TimeoutError:
raise LibraryNotReadyError("Library is not ready yet.")
| await asyncio.wait_for(self.LibraryReadyEvent.wait(), timeout=timeout) | ||
| except asyncio.TimeoutError: | ||
| raise LibraryNotReadyError("Library is not ready yet.") | ||
| self._ensure_ready() |
There was a problem hiding this comment.
I would probably do something like this instead, so we keep waiting until self.is_ready() is actually true, while still respecting the original timeout.
async def wait_for_library_ready(self, timeout: int = None):
if timeout is None:
timeout = self.LibraryReadyTimeout
loop = asyncio.get_running_loop()
deadline = loop.time() + timeout
while True:
if self.is_ready():
return
remaining = deadline - loop.time()
if remaining <= 0:
raise LibraryNotReadyError("Library is not ready yet.")
try:
await asyncio.wait_for(self.LibraryReadyEvent.wait(), timeout=remaining)
except asyncio.TimeoutError:
raise LibraryNotReadyError("Library is not ready yet.")
2 participants
Mithun showed me there is a race condition in the waiting mechanism.
I believe it. It is just still quite difficult to me to understand it fully. Robot summarizes:
wait_for_library_ready() waited only on LibraryReadyEvent, which behaves like a doorbell: it signals that readiness happened at some point (set() on the not-ready → ready edge). open() / list() need the stronger guarantee: the library is ready right now — like checking through the peephole before you act.
Without a check after the await, another task could move the library back to not-ready between “the bell rang” and “this coroutine continues,” so callers could proceed on a stale observation.
Summary by CodeRabbit