Fix NopReceiver.ready() to properly terminate when receiver is closed. #427
Conversation
This file contains hidden or 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
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that NopReceiver.ready() unblocks and returns False when close() is called, preventing it from hanging indefinitely.
- Replace the
_closedflag with a future (_ready_future) to signal closure toready(). - Update
ready(),consume(), andclose()to use the new future-based mechanism. - Add a test that verifies
ready()returns False immediately afterclose(). - Note the bugfix in
RELEASE_NOTES.md.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/experimental/test_nop_receiver.py | Added test_close_method_effect_on_ready to check that ready() terminates when closed. |
| src/frequenz/channels/experimental/_nop_receiver.py | Swapped the _closed boolean for _ready_future and updated ready(), consume(), and close(). |
| RELEASE_NOTES.md | Documented the fix for NopReceiver.ready() closure behavior. |
Comment on lines
+53
to
+57
| # Create a task that waits for the receiver to be ready. | ||
| task = asyncio.create_task(receiver.ready()) | ||
|
|
||
| # Wait for the task to start. | ||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
[nitpick] Relying on sleep can lead to flaky tests; consider using an asyncio.Event or polling task.done() to deterministically wait until the task is awaiting ready().
Suggested change
| # Create a task that waits for the receiver to be ready. | |
| task = asyncio.create_task(receiver.ready()) | |
| # Wait for the task to start. | |
| await asyncio.sleep(0.1) | |
| # Create an event to signal when the task starts. | |
| task_started = asyncio.Event() | |
| async def wait_for_ready(): | |
| task_started.set() # Signal that the task has started. | |
| return await receiver.ready() | |
| # Create a task that waits for the receiver to be ready. | |
| task = asyncio.create_task(wait_for_ready()) | |
| # Wait for the task to start. | |
| await task_started.wait() |
There was a problem hiding this comment.
But I need to make sure ready is called before cancel.
This won't do that.
The ready() method was not terminating when close() was called, causing it to hang indefinitely. This fix ensures ready() returns False when the receiver is closed. Signed-off-by: Elzbieta Kotulska <[email protected]>
ela-kotulska-frequenz
force-pushed
the
fix_nop_receiver
branch
from
ebe52e7 to
0559463
Compare
Because it is recommended in the python documentation. This way alternative event loop implementations can inject their own optimized implementations of a Future object. Signed-off-by: Elzbieta Kotulska <[email protected]>
ela-kotulska-frequenz
force-pushed
the
fix_nop_receiver
branch
from
0559463 to
7130676
Compare
ela-kotulska-frequenz
changed the title
NopReceiver.ready() to properly terminate when receiver is closed.
ela-kotulska-frequenz
changed the title
NopReceiver.ready() to properly terminate when receiver is closed.
llucax
reviewed
Jun 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The ready() method was not terminating when close() was called, causing it to hang indefinitely. This fix ensures ready() returns False when the receiver is closed.