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

Consider replacing channels #1907

Closed
semtexzv opened this issue Oct 9, 2019 · 6 comments
Closed

Consider replacing channels #1907

semtexzv opened this issue Oct 9, 2019 · 6 comments
Labels
A-channel Area: futures::channel

Comments

@semtexzv
Copy link

semtexzv commented Oct 9, 2019

Current implementation can be traced back to the standard library. It worked great for a while, but it seems, it would be wise to replace this aging code with something new while we still have a chance, before stabilizing 0.3.

Issues with current implementation:

  • Not-quite bounded: The current implementation of channels reserves a slot for each cloned sender. When using this with async-await, the common practice to ensure a future is Send + 'static is cloning the sender, and using a cloned variant. This basically makes bounded channels unbounded, and thus useless.
  • Tree-like queue: Current implementation of both bounded channels uses a tree-like queue, which could be replaced by lock-free ring buffer. I suspect a ring-buffer like structure would provide better cache behavior for cases when channel is not full, but provide some cache contention for full channels ( citation needed , just my hunch)
  • Race condition in futures::sync::mpsc #909
  • Surprising behavior from mpsc channels when only using try_ methods #861
  • many others

Replacements:

Motivation

The futures-rs crate is backbone of the async ecosystem. The proliferation of alternative channel implementations signifies that this aspect of the library no longer serves the needs of the community. In order to resolve these issues, a new implementation of channels is needed.

There is one extremely good implementation of channels in the wild, which is in consideration for inclusion in the standard library, and the futures library is getting left behind by these improvements.

@djc
Copy link

djc commented Oct 9, 2019

Alternatively:

  • Strip channels out of futures, and leave it to another party (potentially crossbeam-channels itself) to provide Future and/or Stream-enabled channels?

  • For now, only ship channels as part of a separate crate, so that the implementation will potentially be easier to deprecate later on.

@Nemo157
Copy link
Member

Nemo157 commented Oct 9, 2019

futures-channel is already a separate crate, depended on only by futures-util and futures

@semtexzv
Copy link
Author

semtexzv commented Oct 9, 2019

Yes, these are valid approaches to making channels more replaceable down the road. But my issue is:

  • They are broken
  • We can do something about them right now, since we are in the process of transitioning to 0.3,
    So why don't we ?

There is an associated question/implicit assumption: Are the channels necessary part of the futures crate ? I'd say yes, because I see futures crate as a first one I include inside a project that utilizes std::futures + async await. And a channel is an abstraction that is invaluable. So I will go from this assumption forward.

After taking in these assumptions, the logical question is: With what ? And there are several possible answers, 2 of which are in my original post

@Nemo157
Copy link
Member

Nemo157 commented Oct 9, 2019

The only internal dependency on channels is the oneshot channel being used for the futures::future::RemoteHandle implementation, and a whole lot of examples using channels.

I agree that it makes sense for futures to come with a good set of channels, I find them much more prevalent in async code than sync code.

@carllerche
Copy link
Member

For some context, I contributed the original mpsc implementation in futures-rs many years ago. It was always intended to be a "first pass". Since then, I wrote the "next generation" channel as part of tokio-sync. The primary reason for releasing it as part of tokio-sync vs. contributing it here was ease. Its easier for me to land things in tokio than it is here. There still are more improvements that can be made to Tokio's channel.

Even if futures provides its own set of channels, I expect there to be value in Tokio maintaining its own implementation that has better optimized to the details of its runtime.

@Matthias247
Copy link
Contributor

If you are interested in another approach on implementing channels, you might want to take a look at (or even use) the futures-intrusive implementations.

Those have the advantage of being truly bounded (even unbuffered/rendezvous is possible), are MPMC and don’t require extra resources per using task. They are also somewhat simpler (less atomic magic).

An advantage that wasn’t yet mentioned here is that it doesn’t leak Wakers (which can lead to a memory leak) when tasks are cancelled.

Performance aspect depends on the use case. I did some benchmarking and included ones in the repo. I looks to me like it rather performs better than worse for typical applications, but in doubt everyone can do their own evaluation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-channel Area: futures::channel
Projects
None yet
Development

No branches or pull requests

6 participants