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::watch: Add has_changed flag and method to Ref #4758

Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 48 additions & 6 deletions tokio/src/sync/watch.rs
Expand Up @@ -335,12 +335,11 @@ impl<T> Receiver<T> {
Ref { inner }
}

/// Returns a reference to the most recently sent value and mark that value
/// Returns a reference to the most recently sent value and marks that value
/// as seen.
///
/// This method marks the value as seen, so [`changed`] will not return
/// immediately if the newest value is one previously returned by
/// `borrow_and_update`.
/// This method marks the current value as seen. Subsequent calls to [`changed`]
/// will not return immediately until the producer has modified the shared value again.
///
/// Outstanding borrows hold a read lock. This means that long lived borrows
/// could cause the send half to block. It is recommended to keep the borrow
Expand All @@ -365,9 +364,52 @@ impl<T> Receiver<T> {
///
/// [`changed`]: Receiver::changed
pub fn borrow_and_update(&mut self) -> Ref<'_, T> {
self.borrow_and_update_if_changed().0
}

/// Returns a reference to the most recently sent value with an *updated*
/// flag indicating if that value has been detected as *changed* and marked
/// as seen.
///
/// This method marks the current value as seen. Subsequent calls to [`changed`]
/// will not return immediately until the producer has modified the shared value again.
///
/// Outstanding borrows hold a read lock. This means that long lived borrows
/// could cause the send half to block. It is recommended to keep the borrow
/// as short lived as possible.
///
/// The priority policy of the lock is dependent on the underlying lock
/// implementation, and this type does not guarantee that any particular policy
/// will be used. In particular, a producer which is waiting to acquire the lock
/// in `send` might or might not block concurrent calls to `borrow`, e.g.:
///
/// <details><summary>Potential deadlock example</summary>
///
/// ```text
/// // Task 1 (on thread A) | // Task 2 (on thread B)
/// let (_ref1, _updated1) = |
/// rx1.borrow_and_update_if_changed(); |
/// | // will block
/// | let _ = tx.send(());
/// // may deadlock |
/// let (_ref2, _updated2) = |
/// rx2.borrow_and_update_if_changed(); |
/// ```
/// </details>
///
/// [`changed`]: Receiver::changed
pub fn borrow_and_update_if_changed(&mut self) -> (Ref<'_, T>, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

here's a minor API design suggestion. what if, instead of a tuple of Ref and bool, we added the bool to ref along with a Ref::changed() method. that way, the meaning of the boolean value would be clearer at the callsite, as opposed to making the user responsible for naming the tuple field. it would also negate the need to have two separate methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a good way to go around it.

Copy link
Contributor Author

@uklotzde uklotzde Jun 8, 2022

Choose a reason for hiding this comment

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

Good point! I was already experimenting with such a design in my external wrapper crate. Just wanted to avoid introducing new, named types.

I didn't consider the fact that this solutions enables us to extend the current API without any breaking changes, which makes it even more appealing :) Thank you for the hint!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was blind 🙈 Your proposal turned out as the perfect solution that only affects borrow_and_update() when focusing on the updated aspect and not considering the more general changed aspect: 63c9987

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that commits will be squashed before merging, so I added the changes as a new commit.

let inner = self.shared.value.read().unwrap();
self.version = self.shared.state.load().version();
Ref { inner }
// After obtaining a read-lock no concurrent writes could occur
// and the loaded version matches that of the borrowed reference.
let new_version = self.shared.state.load().version();
let updated = if self.version != new_version {
self.version = new_version;
true
} else {
false
};
(Ref { inner }, updated)
}

/// Checks if this channel contains a message that this receiver has not yet
Expand Down