sync: improve docs for watch channels #5954

sunshowers · 2023-08-27T00:36:10Z

Motivation

I found the watch docs as written to be somewhat confusing.

It wasn't clear to me whether values are marked seen or not at creation/subscribe time.
The example also confused me a bit, suggesting a while loop when a do-while loop is generally more correct.
I noticed a potential race with borrow that is no longer an issue with borrow_and_update.

Solution

Update the documentation for the watch module to try and make all this clearer.

I found the watch docs as written to be somewhat confusing. * It wasn't clear to me whether values are marked seen or not at creation/subscribe time. * The example also confused me a bit, suggesting a while loop when a do-while loop is generally more correct. * I noticed a potential race with `borrow` that is no longer an issue with `borrow_and_update`. Update the documentation to try and make it clearer.

sunshowers · 2023-08-27T06:24:13Z

tokio/src/sync/watch.rs

+//! use [`Receiver::borrow_and_update()`].
+//!
+//! To access the latest value but **not** mark it as seen, use
+//! [`Receiver::borrow()`].


Might want to mention here that if the value has already been seen its state doesn't change.

Darksonn

This looks reasonable to me.

Darksonn · 2023-08-27T15:14:09Z

tokio/src/sync/watch.rs

+//! * On completion, the [`changed`] method marks the new value as *seen*. If
+//!   [`Receiver::changed()`] is called again, it will not be ready unless a
+//!   subsequent value is sent.


This sentence is a bit confusing. Can we rephrase "not be ready" to "not return immediately" or similar?

Done, thanks!

Darksonn

Only minor nits. Overall looks good!

Darksonn · 2023-08-28T13:56:22Z

tokio/src/sync/watch.rs

+//! * The [`changed`] method returns `Ok(())` on receiving a new value, or
+//!   `Err(_)` if the [`Sender`] has been closed.
+//! * On completion, the [`changed`] method marks the new value as *seen*. If
+//!   [`Receiver::changed()`] is called again, it will not return immediately
+//!   unless a subsequent value is sent.


Perhaps it would make sense to add this point somewhere here?

If the latest value is unseen when calling changed, then changed will return immediately. If the latest message is seen, then it will sleep until a new message is sent.

I don't think the current way its written make this sufficiently clear.

Then the second point can also be simplified to this:

On completion, the changed method marks the new value as seen.

+1 to @Darksonn's comment. Also, perhaps some of this documentation should also be copied into the Receiver::changed() docs?

I like this a lot. Updated with this + also a mention that the sender can be dropped.

hawkw

Overall, this looks great, thanks for working on it! I commented on some very minor nitpicks and edits, let me know what you think!

tokio/src/sync/watch.rs

hawkw · 2023-08-28T17:58:28Z

tokio/src/sync/watch.rs

+//! * The [`changed`] method returns `Ok(())` on receiving a new value, or
+//!   `Err(_)` if the [`Sender`] has been closed.
+//! * On completion, the [`changed`] method marks the new value as *seen*. If
+//!   [`Receiver::changed()`] is called again, it will not return immediately
+//!   unless a subsequent value is sent.


+1 to @Darksonn's comment. Also, perhaps some of this documentation should also be copied into the Receiver::changed() docs?

tokio/src/sync/watch.rs

hawkw · 2023-08-28T18:00:37Z

tokio/src/sync/watch.rs

+//! To access the **latest** value stored in the channel and mark it as *seen*,
+//! use [`Receiver::borrow_and_update()`].


do you think it's worth explicitly stating something like "borrow_and_update() should be preferred if the receiver intends to await notifications from changed()"?

I kinda added that to the examples section but maybe I should move it up to this section.

Moved it into a new section titled "borrow_and_update versus borrow". Hopefully that works.

hawkw · 2023-08-28T18:01:13Z

tokio/src/sync/watch.rs

+//! [`Receiver::borrow()`]. (If the value has already been marked *seen*,
+//! [`Receiver::borrow()`] is equivalent to [`Receiver::borrow_and_update()`].)


do you think it's worth explicitly stating something like "borrow_and_update() should be preferred if the receiver intends to await notifications from changed(), but it is not required if the receiver is just looking at the current value of the shared state without waiting for changes"?

Added this to the new section, and also mentioned why borrow can be more convenient (it's &self).

do you think we should maybe add a

Suggested change

//! [`Receiver::borrow()`]. (If the value has already been marked *seen*,

//! [`Receiver::borrow()`] is equivalent to [`Receiver::borrow_and_update()`].)

//! [`Receiver::borrow()`]. (If the value has already been marked *seen*,

//! [`Receiver::borrow()`] is equivalent to [`Receiver::borrow_and_update()`].)

//!

//! See [here](#borrow-and-update-versus-borrow) for more information on when

//! to use these methods.

or something?

hawkw · 2023-08-28T18:03:29Z

tokio/src/sync/watch.rs

+//! method is ready when a new, *unseen* value is sent via the [`Sender`] half.
+//!
+//! * The [`changed`] method returns `Ok(())` on receiving a new value, or
+//!   `Err(_)` if the [`Sender`] has been closed.


very tiny nit, not actually important: this returns a public named error type, so we could actually reference it:

Suggested change

//! `Err(_)` if the [`Sender`] has been closed.

//! `Err(`[`errors::RecvError`]`)` if the [`Sender`] has been closed.

sunshowers · 2023-08-28T19:41:01Z

Done, thanks for the comments.

hawkw

looks great to me overall! i'd like to see links back to the section on "borrow_and_update versus borrow" in the method docs for those methods --- mind adding that?

hawkw · 2023-08-28T19:50:10Z

tokio/src/sync/watch.rs

+//! ## `borrow_and_update` versus `borrow`
+//!
+//! If the receiver intends to await notifications from [`changed`] in a loop,
+//! [`Receiver::borrow_and_update()`] should be preferred over
+//! [`Receiver::borrow()`].  This avoids a potential race where a new value is
+//! sent between [`changed`] being ready and the value being read. (If
+//! [`Receiver::borrow()`] is used, the loop may run twice with the same value.)
+//!
+//! If the receiver is only interested in the current value, and does not intend
+//! to wait for changes, then [`Receiver::borrow()`] can be used. It may be more
+//! convenient to use [`borrow`](Receiver::borrow) since it's an `&self`
+//! method---[`borrow_and_update`](Receiver::borrow_and_update) requires `&mut
+//! self`.


this is fantastic --- but i think the doc comments for borrow and borrow_and_update probably ought to link back to this section. :)

tokio/src/sync/watch.rs

hawkw · 2023-08-28T19:52:05Z

tokio/src/sync/watch.rs

+//! [`Receiver::borrow()`]. (If the value has already been marked *seen*,
+//! [`Receiver::borrow()`] is equivalent to [`Receiver::borrow_and_update()`].)


do you think we should maybe add a

Suggested change

//! [`Receiver::borrow()`]. (If the value has already been marked *seen*,

//! [`Receiver::borrow()`] is equivalent to [`Receiver::borrow_and_update()`].)

//! [`Receiver::borrow()`]. (If the value has already been marked *seen*,

//! [`Receiver::borrow()`] is equivalent to [`Receiver::borrow_and_update()`].)

//!

//! See [here](#borrow-and-update-versus-borrow) for more information on when

//! to use these methods.

or something?

sunshowers · 2023-08-28T20:12:42Z

Added links. I think this is ready to go now. Thanks!

hawkw

looks great to me!

github-actions bot added the R-loom-sync Run loom sync tests on this PR label Aug 27, 2023

hawkw self-requested a review August 27, 2023 00:37

sunshowers commented Aug 27, 2023

View reviewed changes

Darksonn reviewed Aug 27, 2023

View reviewed changes

Darksonn added T-docs Topic: documentation A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Aug 27, 2023

sunshowers added 2 commits August 27, 2023 12:28

address review comment

6d7ec40

also make it clearer that borrow doesn't change its state

0b7f413

Darksonn reviewed Aug 28, 2023

View reviewed changes

address review comment

7091060

hawkw reviewed Aug 28, 2023

View reviewed changes

more review comments

cc0433d

hawkw approved these changes Aug 28, 2023

View reviewed changes

add links to borrow_and_update-versus-borrow

4caf956

hawkw approved these changes Aug 28, 2023

View reviewed changes

hawkw merged commit cb1e10b into tokio-rs:master Aug 28, 2023
70 checks passed

satakuma mentioned this pull request Oct 8, 2023

chore: prepare Tokio v1.33.0 release #6059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync: improve docs for watch channels #5954

sync: improve docs for watch channels #5954

sunshowers commented Aug 27, 2023

sunshowers Aug 27, 2023

Darksonn left a comment

Darksonn Aug 27, 2023

sunshowers Aug 27, 2023

Darksonn left a comment

Darksonn Aug 28, 2023

hawkw Aug 28, 2023

sunshowers Aug 28, 2023

hawkw left a comment

hawkw Aug 28, 2023

hawkw Aug 28, 2023

sunshowers Aug 28, 2023

sunshowers Aug 28, 2023

hawkw Aug 28, 2023

sunshowers Aug 28, 2023

hawkw Aug 28, 2023

hawkw Aug 28, 2023

sunshowers Aug 28, 2023

sunshowers commented Aug 28, 2023

hawkw left a comment

hawkw Aug 28, 2023

sunshowers Aug 28, 2023

hawkw Aug 28, 2023

sunshowers commented Aug 28, 2023

hawkw left a comment

		//! To access the latest value stored in the channel and mark it as seen,
		//! use [`Receiver::borrow_and_update()`].

		//! [`Receiver::borrow()`]. (If the value has already been marked seen,
		//! [`Receiver::borrow()`] is equivalent to [`Receiver::borrow_and_update()`].)

	//! `Err(_)` if the [`Sender`] has been closed.
	//! `Err(`[`errors::RecvError`]`)` if the [`Sender`] has been closed.

sync: improve docs for watch channels #5954

sync: improve docs for watch channels #5954

Conversation

sunshowers commented Aug 27, 2023

Motivation

Solution

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Aug 28, 2023

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Aug 28, 2023

hawkw left a comment

Choose a reason for hiding this comment