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

sync: elaborate on unbounded send not being async #2600

Merged
merged 4 commits into from
Jun 17, 2020

Conversation

Darksonn
Copy link
Contributor

I keep seeing people being confused about the unbounded send method not being async, and thinking that our channels are actually blocking. I hope that this snippet improves on this.

@Darksonn Darksonn added T-docs Topic: documentation C-maintenance Category: PRs that clean code up or issues documenting cleanup. A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Jun 10, 2020
@Darksonn Darksonn self-assigned this Jun 10, 2020
@Darksonn
Copy link
Contributor Author

It seems like the macos CI fix didn't work? @taiki-e

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This looks good to me!

What do you think about also adding a similar note in the top-level docs for mpsc? We mention the existence of the unbounded channel here:

//! Unbounded channels are also available using the `unbounded_channel`
//! constructor.

but never really explain what that means. It might be helpful for new users to highlight the key difference between the bounded and unbounded channel's APIs?

tokio/src/sync/mpsc/unbounded.rs Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented Jun 10, 2020

It seems like the macos CI fix didn't work?

Oops, It seems I've only fixed the azure pipelines.
It should be fixed in #2602.

@Darksonn Darksonn requested a review from hawkw June 10, 2020 21:29
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Thanks for adding to the module level docs! I had some minor suggestions, but they aren't blockers.

tokio/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
//! **Bounded channel**: If you need a bounded channel, you should use a bounded
//! Tokio mpsc channel for both directions of communication. To call the async
//! [`send`][bounded-send] or [`recv`][bounded-recv] methods in sync code, you
//! should use [`Handle::block_on`].
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like:

Suggested change
//! should use [`Handle::block_on`].
//! should use [`Handle::block_on`]. This is because the sender must be able to wait
//! for additional capacity when the channel is full. `Handle::block_on` allows a
//! synchronous caller to wait on an `async fn`.

(in general, I want to make sure all docs that state "you should do X" also explain why you should do that)

tokio/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor Author

I added a similar comment to the oneshot channel.

@Darksonn Darksonn requested a review from hawkw June 11, 2020 08:47
@Darksonn Darksonn merged commit 3db22e2 into tokio-rs:master Jun 17, 2020
@Darksonn Darksonn deleted the unbounded-is-not-blocking branch June 17, 2020 20:15
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me! i had a few nits.

@@ -10,8 +10,13 @@
//! is rejected and the task will be notified when additional capacity is
//! available. In other words, the channel provides backpressure.
//!
//! Unbounded channels are also available using the `unbounded_channel`
//! constructor.
//! This module provides two variants of the channel: A bounded and an unbounded
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
//! This module provides two variants of the channel: A bounded and an unbounded
//! This module provides two variants of the channel: a bounded and an unbounded

Comment on lines +13 to +14
//! This module provides two variants of the channel: A bounded and an unbounded
//! variant. The bounded variant has a limit on the number of messages that the
Copy link
Member

Choose a reason for hiding this comment

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

also, how about just

Suggested change
//! This module provides two variants of the channel: A bounded and an unbounded
//! variant. The bounded variant has a limit on the number of messages that the
//! This module provides two variants of the channel: bounded and unbounded.
//! The bounded variant has a limit on the number of messages that the

//! This module provides two variants of the channel: A bounded and an unbounded
//! variant. The bounded variant has a limit on the number of messages that the
//! channel can store, and if this limit is reached, trying to send another
//! message will sleep until a message is received from the channel. An unbounded
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

Suggested change
//! message will sleep until a message is received from the channel. An unbounded
//! message will wait until a message is received from the channel. An unbounded

rather than "sleep", for consistency?

hawkw added a commit that referenced this pull request Jul 20, 2020
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2572, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)

- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- io: fix panic in `AsyncReadExt::read_line` (#2541)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 22, 2020
# 0.2.22 (July 2!, 2020)

### Fixes
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)
- io: fix panic in `AsyncReadExt::read_line` (#2541)

### Changes
- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

### Added
- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-sync Module: tokio/sync T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants