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

Add top-level channel module? #445

Closed
matklad opened this issue Nov 3, 2019 · 7 comments
Closed

Add top-level channel module? #445

matklad opened this issue Nov 3, 2019 · 7 comments
Labels
api design Open design questions feedback wanted Needs feedback from users

Comments

@matklad
Copy link
Member

matklad commented Nov 3, 2019

Currently, channel, Sender and Receiver live directly in async_std::sync. This doesn't match std (where there's a sync::mpsc submodule) and looks pretty weird on it's own:

  • I expect sync::channel to be a module (to the point that I was thinking about it as such when writing a title for this issue, despite the fact that I've used this function a minute ago)
  • Knowing that sync::channel is a function, I need to refer to example to recall where should I find Sender and Receiver types.

Top-level async_std::channel would make more sense to me personally. It diverges from the standard tradition of piling everything into the sync module, but it is rumored that std::sync might be split itself.

@ghost
Copy link

ghost commented Nov 3, 2019

So the reason why I chose not to have channel module is twofold.

First, the channel API is no bigger than the RW lock API. In fact, all of the synchronization primitives come in multiple types:

  • Arc, Weak
  • Mutex, MutexGuard
  • Barrier, BarrierWaitResult
  • channel, Sender, Receiver
  • RwLock, RwReadGuard, RwWriteGuard

If channel deserves to be its own module, why doesn't RwLock?

Second, what do we name the channel module - mpmc, channel, chan? What do we name the constructor - new, bounded, channel? All combinations of these felt "meh" to me, but I wonder which combination you prefer?

Top-level async_std::channel would make more sense to me personally. It diverges from the standard tradition of piling everything into the sync module, but it is rumored that std::sync might be split itself.

I would actually like std::sync to be split. :) And the name "sync" has always been comfusing to me.

cc @yoshuawuyts Opinions?

@matklad
Copy link
Member Author

matklad commented Nov 3, 2019

I think all other things in sync have one central type, and a number of subordinate types (its even reflected in the naming of all things except Arc/Weak), while for channels all of channel, Sender, Receiver are equally important.

For naming channel { bounded } or channel { channel } both feel ok to me. I definitely don't like mpmc. Maybe we even need channel { channel, bounded }, where channel is a zero-arg function which creates channel with default capacity (1), and bounded is what today is channel? This is a separate issue really, but I don't like how current API forces you to pick some capacity on every call-site.

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Nov 3, 2019

For me a guiding design principle is to follow std's lead on module hierarchy and naming. The split hasn't been even formally proposed yet, and even if it would happen the sync submodule will not go away — at most it'd be deprecated.

Given std hasn't set a precedent yet, I don't think we should create a new submodule for channels.

note: I agree this is not entirely black/white since our channel type has no counterpart in std. But introducing new top-level modules feels like a bigger step than a new API, or porting a convention (crossbeam-channels is a defacto replacement). Ideally I'd like to eventually also see an mpsc channel in async-std for porting compat purposes only. But like, that's not a priority.

@matklad
Copy link
Member Author

matklad commented Nov 3, 2019

Note that if we strictly follow conventions of std, we should put channels into sync::mpmc submodule, and not directly into sync.

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Nov 3, 2019

@matklad if mpmc channels ever made it into std, following this proposal mpsc channels would be deprecated. The reason for that is because it'd be a chance to also streamline the somewhat confusing terminology that is considered a mis-design of mpsc channels.

That's why I mentioned this is a bit more murky because this is a new API that doesn't quite have precedence in std. But creating a new submodule feels like a bigger leap than how introducing channel. I agree it's somewhat vague, and hard to quantify, but the difference between introducing a new submodule, and introducing a new type in a submodule feels significant.

@ghost ghost added api design Open design questions feedback wanted Needs feedback from users labels Nov 3, 2019
@dignifiedquire
Copy link
Member

I would strongly prefer to have the current channel be renamed to channel::bounded. Even if there are no unbounded channels implemented right now, it is quite confusing to only find a single type of channel.

@yoshuawuyts
Copy link
Contributor

Closed by #827 which adds a top-level channel submodule

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api design Open design questions feedback wanted Needs feedback from users
Projects
None yet
Development

No branches or pull requests

3 participants