Repository navigation
Conversation
The experimental `Pipe` leaves its sender and its receiver open when it stops by itself, so every user has to work out how to close them, and a failed send ends its task with an exception nobody reads. Fixing that in place would change the behavior of an existing class under users that may rely on it, so add the new behavior as a separate `Pipe` at the top level, and let users move to it when they have checked how they close their endpoints. The new `Pipe` closes its sender when its receiver stops, and closes both its sender and its receiver when sending fails with a `SenderError`, logging the failure. With `close_pipe_on_send_error=False` it drops the message that failed and keeps forwarding the next ones. `stop()` and any other exception leave both ends open. This is the `Pipe` from frequenz-floss#565, which changed the experimental one in place on `v2.x.x`. It doesn't use `NoReceiversError`, which only exists on `v2.x.x`: it only picked the log level there, so here every `SenderError` is logged as an error with its traceback. The tests are ported accordingly, as `BroadcastChannel` in v1 closes itself when it is sent a message without receivers, instead of staying open. The experimental `Pipe` is deprecated in the next commit. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The new `Pipe` replaces the experimental one, but it isn't a drop-in replacement: it closes its sender when its receiver stops, and both ends when sending fails, while the experimental one leaves them open. Users should check how they tear their pipes down before they switch, so the warning and the docstring explain the differences, not only the new name. Keep the experimental `Pipe` as a separate implementation with `@deprecated` instead of making it an alias of the new one, as the two don't behave the same and an alias would change the behavior of existing code silently. The `Deprecated:` notice is kept to a pointer to the replacement and what to check, and the full list of differences goes in the docstring body, as the deprecations guide asks. The existing tests keep checking the behavior of the experimental `Pipe` and now assert that it warns, and a few more document how it leaves its ends open. The experimental `Pipe` is removed in v2.0.0. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
github-actions
Bot
added
part:docs
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block approval of the stable Pipe API and experimental migration changes.
No open issues 🎉
What changed in this PR
Adds a stable frequenz.channels.Pipe with explicit endpoint-closing behavior, allowing projects to migrate before v2 while preserving the experimental implementation’s behavior.
Changes:
- Exports the new
Pipe, with automatic endpoint closure and an option to continue after send failures. - Deprecates the experimental
Pipeand documents migration and endpoint ownership. - Adds tests for stable behavior, error handling, deprecation warnings, and experimental compatibility.
| File | Description |
|---|---|
tests/test_pipe.py |
Tests forwarding, endpoint closure, error handling, and warning-free use. |
tests/experimental/test_pipe.py |
Verifies deprecation warnings and preserved legacy behavior. |
src/frequenz/channels/experimental/_pipe.py |
Adds deprecation warnings and migration guidance. |
src/frequenz/channels/_pipe.py |
Implements the stable pipe and documents endpoint ownership. |
src/frequenz/channels/__init__.py |
Exports the stable Pipe publicly. |
RELEASE_NOTES.md |
Announces the new API and experimental deprecation. |
docs/user-guide/utilities/pipes.md |
Adds a user-guide page for the stable pipe. |
Review effort: Balanced
Contributor
Author
|
I’m closing this PR and deferring the new top-level The goal was to let users adopt the new When a
Adding This replaces the plan in #583 to introduce the new |
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
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.
Add the
Pipefrom #565 asfrequenz.channels.Pipe, so projects can switch to its closing behavior while they still support both v1 and v2, and deprecatefrequenz.channels.experimental.Pipe, to remove it in v2.0.0 (#365).The two are separate implementations, not an alias, because they behave differently: the new
Pipecloses its sender when its receiver stops, and closes its sender and its receiver when sending fails with aSenderError(unlessclose_pipe_on_send_error=False, which drops the message and keeps going). The experimental one leaves both ends open. The deprecation warning, the docstring and the release notes explain this, so users can check their teardown before switching.NoReceiversErroronly exists onv2.x.x, so here everySenderErroris logged as an error; v2 only uses it to pick a quieter log level.Fixes #583