Skip to content

Add the oneshot channel implementation#502

Open
shsms wants to merge 3 commits intofrequenz-floss:v1.x.xfrom
shsms:oneshot
Open

Add the oneshot channel implementation#502
shsms wants to merge 3 commits intofrequenz-floss:v1.x.xfrom
shsms:oneshot

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented Mar 5, 2026

No description provided.

Copilot AI review requested due to automatic review settings March 5, 2026 14:44
@shsms shsms requested a review from a team as a code owner March 5, 2026 14:44
@shsms shsms requested review from ela-kotulska-frequenz and removed request for a team March 5, 2026 14:44
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:channels Affects channels implementation part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:experimental Affects the experimental package labels Mar 5, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new oneshot channel implementation to the frequenz.channels library, along with a Sender.aclose() abstract method and SenderClosedError exception class. A oneshot channel allows sending exactly one message, after which the sender and receiver are automatically closed.

Changes:

  • Introduces make_oneshot() factory function and supporting internal _OneShot, _OneShotSender, and _OneShotReceiver classes in a new _oneshot.py module.
  • Adds an abstract aclose() method to the Sender ABC and a new SenderClosedError exception, with implementations in all existing Sender subclasses (Anycast._Sender, Broadcast._Sender, RelaySender).
  • Adds tests for the oneshot channel and updates exports and release notes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/frequenz/channels/_oneshot.py New file implementing the oneshot channel with sender, receiver, and factory function
src/frequenz/channels/_sender.py Adds abstract aclose() method to Sender ABC and new SenderClosedError exception class
src/frequenz/channels/__init__.py Exports make_oneshot and SenderClosedError
src/frequenz/channels/_anycast.py Adds aclose() and SenderClosedError support to Anycast sender
src/frequenz/channels/_broadcast.py Adds aclose() and SenderClosedError support to Broadcast sender
src/frequenz/channels/experimental/_relay_sender.py Adds aclose() to RelaySender, removes redundant typing.Generic base class
tests/test_oneshot.py New test file covering oneshot send/receive, close, and error scenarios
RELEASE_NOTES.md Documents the new oneshot channel feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@shsms shsms force-pushed the oneshot branch 4 times, most recently from 3f0b85a to d8d258f Compare March 5, 2026 15:06
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
from ._sender import Sender, SenderClosedError


def make_oneshot(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open_oneshot_channel()? 😬

I feel pretty strongly about this one. This is not a Trio-only thing, open is also used in asyncio, like open_connection().

I would also be up for open_channel() and take the type as an argument somehow, but I guess it will make the interface more complicated (it will need more type parameters) with very little gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually inclined to switching back to channels.oneshot and channels.broadcast.

IMHO, open doesn't work for us at all. For files and networking, open is an idiomatic verb. In both those cases, there is a counterparty or a resource owner, and the idea is to open something to access the other side.

What we are doing is more like creating, because we are creating both sides and the link between them. So to me, Trio's naming is imprecise.

I'd take inspiration from os.pipe, which is so much closer to what we're doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would also match Rust, but I'm ok with make_oneshot, etc. as well, and it also somewhat similar to the Go syntax, where you have to call make for many things, including channels, a bit like the new keyword of C++.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oneshot for me is not an option because it is not a verb. That leaves us with make or open. Let's say we forget about trio and asyncio, I don't understand why you want to introduce an asymmetry between make/close instead of going with open.

And you are opening a channel, the fact that you hide the channel object l, and you close it indirectly by closing all it's sender's or all it's receivers doesn't change the fact that you are opening a channel that needs to be closed.

And I don't get why you want to follow other programming languages naming conventions in python. I really think is not very relevant what rust or go does here.

I think os.pipe is not a good example because it follows very old POSIX naming conventions, not python ones.

I think open_oneshot_channel is the most precise, descriptive and pythonic option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you want to introduce an asymmetry between make/close instead of going with open.

Only because we'd be sacrificing accuracy in favour of symmetry, which doesn't sound like a good idea. We don't have to open a thing to close it. We just need the thing to be open so that we can close it. And that's what the method would do.

When you create a channel, you get the two open ends of it, that you can close, because when they are created, they are in the open state.

To "open" a thing, its previous state needs to be "closed". To "make" or "create" a thing, it shouldn't exist previously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About make vs create, I think you are just trolling now 😆 .

Create is much more widely used. We don't use make anywhere that I'm aware of, and there is no built-in native1 python function using it, while there are 42 create_*() (including many in asyncio (asyncio.create_task(), asyncio.create_subprocess_exec(), asyncio.create_subprocess_shell(), loop.create_task(), loop.create_connection(), loop.create_datagram_endpoint(), loop.create_server(), loop.create_unix_connection(), loop.create_unix_server(), loop.create_future()).

But again, maybe we can get rid of this problem if we change the interface to create a namedtuple instead and we just call it OneshotChannel.

And if I get more dramatic, considering the main use for this was to replace the channel registry and I think that is not really working great, I'm not sure we really need a oneshot channel at all 😱

Footnotes

  1. As in not inherited from gettex or TK and even counting those there are only a couple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is not really working great

I haven't read your issues in detail yet. Maybe talk about it tomorrow morning?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I though I was serious about naming :D

Maybe I can act as a tie breaker for you. I think that:

  • <verb>_oneshot_channel is the most pythonic pattern. I've noticed that short names, and even abbreviations (e.g. "recv" for receiver) are widely used at Frequenz, which I haven't seen much in past jobs.
  • IMO other languages do not matter unless we plan to migrate this to Rust soon and find it helpful to use Rust's naming conventions already.

And just thinking out loud, have you considered something like this?

sender, receiver = Oneshot.create_sender_and_receiver()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean we need to make the channel class public, but we want to keep it private. But as you might have guessed, I like the explicitness. Maybe to make it a bit shorter it could be create_pair(), and then there is my suggestion above to go with OneshotChannelPair.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I though I was serious about naming :D

Yeah, I have mixed feelings about it, for one side it feels irrelevant, for the other hand is a bit of time invested to get a clean and clear API, which is hard to change in the future.

simonvoelcker
simonvoelcker previously approved these changes Mar 5, 2026
@shsms
Copy link
Contributor Author

shsms commented Mar 9, 2026

The idea to try with a NamedTuple was a good hint. Didn't need NamedTuple, just derived from tuple and it looks great. Called it Oneshot, to be similar to Broadcast.

Will update Broadcast to support this pattern also.

EDIT: maybe will rename to OneshotChannel and BroadcastChannel, so that we don't break the Broadcast interface. Or we discuss other options.

@llucax
Copy link
Contributor

llucax commented Mar 10, 2026

OK, I have a lot of mixed feelings and now looking at it implemented, for me the interface still looks a bit weird (instantiating a channel giving you 2 things), but I can definitely live with it. To try to move forward, I will make 2 small comments:

  • I think adding the Channel suffix is a good addition (because of the from channels import ... thing, so I hope you do that.
  • A maybe, just maybe, way to make it less weird could be sender, receiver = OneshotChannelPair[int]() (not a lot of existing examples, but there is already a sockerpair() out there).

EDIT: Or just OneshotPair, as it is not really a pair of channels, so I can live without the channel word in there. But

Comment that doesn't help to keep progressing, but for me makes a very good argument 😆

I don't know anymore really. I still think the open_oneshot_channel is the cleanest, because the action is opening a channel, and the result is the creation of a sender and a receiver pair. "create" for me is too literal, "create_channel" sounds like you should get a channel, not a sender and receiver, "create_sender_and_receiver" puts too much weight into the sender and receiver, you are creating a channel here, is not like you can have a "create_sender" or "create_receiver" only, they are both ends of a channel.

@shsms
Copy link
Contributor Author

shsms commented Mar 10, 2026

Awesome, no you were right that having to pass the type as an argument is a big disadvantage with using a function. And I'm really glad that subclassing a tuple worked. We can call them OneshotChannel and BroadcastChannel. I'll just add an example and docs and make the PR ready.

@shsms shsms force-pushed the oneshot branch 2 times, most recently from 50a80ad to 5599497 Compare March 10, 2026 12:42
shsms added 2 commits March 10, 2026 13:50
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@llucax
Copy link
Contributor

llucax commented Mar 10, 2026

OK, this looks good to go to me, but I really wonder if it is the best tool to fix the channel registry issue, and if we don't need it for that, I guess we don't have a use case for it, and if we are talking about deprecating/removing anycast because we don't have a use case for it, I'm a bit hesitant to add new channels types we don't use, so I would hold the merging until we are sure we need it. If we decide it is not the right solution, we can merge it anyway in experimental, this way it is there in case we find a good use for it, but it doesn't get promoted as a main feature of the lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:channels Affects channels implementation part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:docs Affects the documentation part:experimental Affects the experimental package part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants