Skip to content
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

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

xilec
Copy link
Contributor

@xilec xilec commented Mar 7, 2021

Without it, when I read documentation I thought that each message is copied for each consumer.

@zeenix zeenix merged commit 4f78290 into smol-rs:master Mar 26, 2021
@zeenix
Copy link
Member

zeenix commented Mar 26, 2021

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? 🤔

@zeenix
Copy link
Member

zeenix commented Mar 26, 2021

@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.

@taiki-e
Copy link
Collaborator

taiki-e commented Mar 26, 2021

I guess you are talking about broadcast channels: https://docs.rs/tokio/1.4.0/tokio/sync/broadcast/index.html

@zeenix
Copy link
Member

zeenix commented Mar 26, 2021

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?

@xilec
Copy link
Contributor Author

xilec commented Mar 26, 2021

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.

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.

@zeenix
Copy link
Member

zeenix commented Mar 26, 2021

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.

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?

@taiki-e
Copy link
Collaborator

taiki-e commented Mar 26, 2021

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.)

@zeenix
Copy link
Member

zeenix commented Mar 27, 2021

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.

Thanks for sharing your thoughts. I'll see what I can cook up.

(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. I didn't know that tokio::sync can be used indepently of tokio runtime. However, both tokio::sync and futures-intrusive duplicate quite a bit of the smol-rs API (locks and channels etc) and since I'm already using those crates from smol-rs, I'd prefer smol-rs to also provide the one API that's missing.

@zeenix
Copy link
Member

zeenix commented Apr 1, 2021

@taiki-e Here is a PoC of what I had in mind. Basically it create a channel for each receiver and aggregates all senders.

@zeenix
Copy link
Member

zeenix commented Apr 5, 2021

@taiki-e Here is a PoC of what I had in mind. Basically it create a channel for each receiver and aggregates all senders.

@taiki-e could you kindly share your opinions here? I'd like to move it under smol-rs. BTW the tokio's broadcast channel API is not part of the standalone tokio_sync crate.

@taiki-e
Copy link
Collaborator

taiki-e commented Apr 6, 2021

@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.

BTW the tokio's broadcast channel API is not part of the standalone tokio_sync crate.

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"] }

@zeenix
Copy link
Member

zeenix commented Apr 6, 2021

@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.

Right, hence the 3rd option here. I'm guess this means you don't much care for option#2?

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:

Oh interesting. Didn't know that. Thanks.

@yoshuawuyts
Copy link
Member

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.

@taiki-e
Copy link
Collaborator

taiki-e commented Apr 28, 2021

Right, hence the 3rd option here. I'm guess this means you don't much care for option#2?

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).

However, both tokio::sync and futures-intrusive duplicate quite a bit of the smol-rs API (locks and channels etc)

The tokio::sync module is a very small subset of tokio, and futures-intrusive is also a relatively small crate, so I don't think this will cause actual problems. (See also bikeshedder/deadpool#88)

@zeenix
Copy link
Member

zeenix commented Apr 28, 2021

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).

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.

@taiki-e
Copy link
Collaborator

taiki-e commented Apr 28, 2021

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.

@zeenix
Copy link
Member

zeenix commented Apr 28, 2021

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 async-channel and async-lock. I think we've discussed this in detail already. The only direct alternative is broadcaster crate but that doesn't have the desired behavior: sending future gives you an error on channel being full while we want (just like async-channel) it to wait till there is a room on the channel for a new message.

The tokio::sync module is a very small subset of tokio,

  • Still it duplicates async-lock so I'll be reluctant for that reason alone.
  • Even if it's agnostic of tokio's runtime, it has tokio- in the name and people would assume that my crate is tokio-specific. There are also political rifts between runtimes that I don't want to be involved in (I'm sure other users will have similar reservations).
  • the broadcast impl. drops messages when a receiver is slow. That's not ideal.

and futures-intrusive is also a relatively small crate,

That also has the same issues (duplicates async-lock and async-channel) and like the broadcaster crate, the broadcasting API doesn't wait for room to be available in the channel and errors out on capacity reached.

I hope these answers are satisfactory. I'll try and make it work now. Wish me luck that I succeed. :)

@taiki-e
Copy link
Collaborator

taiki-e commented Apr 28, 2021

I hope these answers are satisfactory.

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).

sending future gives you an error on channel being full while we want (just like async-channel) it to wait till there is a room on the channel for a new message.

postage probably provide the behavior you want.

  • the broadcast impl. drops messages when a receiver is slow. That's not ideal.

Do you expect all messages to be fully preserved until all senders and receivers are dropped?

  • Still it duplicates async-lock so I'll be reluctant for that reason alone.

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.

@zeenix
Copy link
Member

zeenix commented Apr 29, 2021

I hope these answers are satisfactory.

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).

I'm glad we can agree. :)

sending future gives you an error on channel being full while we want (just like async-channel) it to wait till there is a room on the channel for a new message.

postage probably provide the behavior you want.

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 Stream. Also, the crate is dependent by only 1 other crate so don't know how trust worthy it is. 🤔 Now I'm just thinking out loud. :)

  • the broadcast impl. drops messages when a receiver is slow. That's not ideal.

Do you expect all messages to be fully preserved until all senders and receivers are dropped?

No, only till all current receivers have received the message.

  • Still it duplicates async-lock so I'll be reluctant for that reason alone.

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.

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).

@taiki-e taiki-e mentioned this pull request Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants