-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added in docs main work principle of channel #37
Conversation
Thanks so much. I actually got the same wrong impression myself from the docs and was very disapointed to learn that it's not the case. :( I wonder if we can provide this feature here or would it make sense to have that as a separate crate? 🤔 |
@taiki-e WDYT? Maybe we could have something like: impl<T: Clone> Sender<T> {
// Send the `msg` to all current consumers.
//
// This method clones the `msg` n - 1 times, where `n` is the number of consumers.
pub fn send_all(&self, msg: T) -> Send<'_, T>;
} Client code can do the same but it's easier we provide this implementation and ensure that number of consumers don't change during this call. |
I guess you are talking about broadcast channels: https://docs.rs/tokio/1.4.0/tokio/sync/broadcast/index.html |
Ah yes, didn't know there is a name for the concept already. :) That is indeed what I'm talking about and actually need in zbus. WDYT of my API idea above? Haven't really thought it through so maybe I'm missing something and it's not feasible to implement? |
Actually, each consumer pull items independently from channel's queue and we can get case where one consumer pulled 2 cloned item, but another consumer missed this item. |
Ah right. As I said, I hadn't thought it through yet. :) I'll see if new broadcast API can be added on top but I hope you don't have any general objection on that API being part of this crate? |
I'm not sure if broadcast can be implemented as an extension of existing APIs, but if it is not implementable as an extension of existing APIs, I think is better to implement it as a separate crate for now. (Btw, tokio::sync is compatible with any runtimes, so I think you can use tokio's broadcast. And IIRC futures-intrusive also provide broadcast.) |
Thanks for sharing your thoughts. I'll see what I can cook up.
Thanks. I didn't know that |
@taiki-e could you kindly share your opinions here? I'd like to move it under |
@zeenix To be honest, I feel the current implementation is like a trick to use a non-broadcast channel as a broadcast channel and I guess is not efficient than a crate originally designed as a broadcast channel.
tokio is currently merging all sub crates (tokio-io, tokio-sync, etc.) into the main tokio crate and is designed for users to enable via feature flags. For example, to use the sync module that contains broadcast channels, enable the sync feature: tokio = { version = "1", features = ["sync"] } |
Right, hence the 3rd option here. I'm guess this means you don't much care for option#2?
Oh interesting. Didn't know that. Thanks. |
Previous discussion about adding broadcast channels on async-std is here: async-rs/async-std#486. We independently arrived at much the same conclusions as @zeenix has in this thread. And I continue to believe that this is the right direction to explore. |
Considering performance, I think 3 is the only option. However, implementing a channel from scratch is not an easy task, so I think we will have to consider whether it is really worth providing our own crate (if provide it as part of smol-rs organization).
The |
Hence picking up broadcast-channel. I'll be adding the async parts soon to it and if all works well in my testing, I think that's good for publishing. |
It is recommended that you not only test it, but also compare and benchmark it against existing implementations before publishing. If there is no advantage over existing implementations, it is not very useful. |
The rationale for the existence is not very different from that of
That also has the same issues (duplicates I hope these answers are satisfactory. I'll try and make it work now. Wish me luck that I succeed. :) |
Thanks for the explanation! Aside from the one about duplication, I think the difference in behavior from the existing implementation (error on full capacity, drops messages, etc.) is enough reason for its crate to exist (if there is no crate with the same behavior).
postage probably provide the behavior you want.
Do you expect all messages to be fully preserved until all senders and receivers are dropped?
Personally, I don't share this as I prefer to use a high-performance implementation over some duplication. (Also, actually, I think that in many cases, some duplication is unavoidable.) But it’s probably a matter of preference. |
I'm glad we can agree. :)
Oh I didn't notice that one. Thanks for pointing it out. It indeed does implement the behavior I (as would many) need and it seems small enough (especially with most features disabled). However, it does duplicate a bit as well. The other bits of duplication are fine but seems they've their own Stream and Sink traits and don't depend on the appropriate futures crates. Perhaps they've good reason for that though. 🤔 A few trait duplications doesn't really matter though but futures is sorta a staging ground for std so duplicating that is a bit suspicious, especially
No, only till all current receivers have received the message.
Definitely very subjective for sure. From my crate's (and I can imagine others being in the same boat) perspective, if it's already tied into smol-rs ecosystem, using another smol-rs crate would be a nobrainer over using something else even if it means a bit of performance penalty (unless of course, it's significant enough in my case to worry about). |
Without it, when I read documentation I thought that each message is copied for each consumer.