-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
sync::watch
: Add has_changed
flag and method to Ref
#4758
Conversation
This function could otherwise not be implemented in terms of the public API, because invoking has_changed() and borrow_and_update() independently is not thread-safe.
80298c6
to
6445c44
Compare
tokio/src/sync/watch.rs
Outdated
/// </details> | ||
/// | ||
/// [`changed`]: Receiver::changed | ||
pub fn borrow_and_update_if_changed(&mut self) -> (Ref<'_, T>, bool) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
The flag is only set by Receiver::borrow_and_update(). Otherwise it is initialized with `false`.
updated
flag and method to Ref
updated
flag and method to Ref
sync::watch
: Add updated
flag and method to Ref
tokio/src/sync/watch.rs
Outdated
/// Indicates if the shared value has been detected as _changed_ | ||
/// **and** marked as seen. | ||
/// | ||
/// The result is `true` only if this reference has been obtained | ||
/// from [`Receiver::borrow_and_update()`] in case changes have been | ||
/// detected and marked as seen. In all other cases the result will | ||
/// be `false`. | ||
pub fn updated(&self) -> bool { | ||
self.updated | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems more useful to have this function return whether the value was marked as seen before the call. Then the value is also useful for the borrow
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mean to
- add an invocation of
load()
toborrow()
and - rename the function to
has_changed()
.
I was unsure if the extra load()
in borrow()
is desired. It probably doesn't have any noticeable effect and a more consistent interface might be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d081f83
to
b4fef98
Compare
sync::watch
: Add updated
flag and method to Ref
sync::watch
: Add has_changed
flag and method to Ref
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
…_update_if_changed'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This looks good to me.
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.19.2` -> `1.20.0` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.19.2` -> `1.20.0` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.20.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.20.0) [Compare Source](tokio-rs/tokio@tokio-1.19.2...tokio-1.20.0) ##### 1.20.0 (July 12, 2022) ##### Added - tokio: add track_caller to public APIs ([#​4772], [#​4791], [#​4793], [#​4806], [#​4808]) - sync: Add `has_changed` method to `watch::Ref` ([#​4758]) ##### Changed - time: remove `src/time/driver/wheel/stack.rs` ([#​4766]) - rt: clean up arguments passed to basic scheduler ([#​4767]) - net: be more specific about winapi features ([#​4764]) - tokio: use const initialized thread locals where possible ([#​4677]) - task: various small improvements to LocalKey ([#​4795]) ##### Fixed ##### Documented - fs: warn about performance pitfall ([#​4762]) - chore: fix spelling ([#​4769]) - sync: document spurious failures in oneshot ([#​4777]) - sync: add warning for watch in non-Send futures ([#​4741]) - chore: fix typo ([#​4798]) ##### Unstable - joinset: rename `join_one` to `join_next` ([#​4755]) - rt: unhandled panic config for current thread rt ([#​4770]) [#​4677]: tokio-rs/tokio#4677 [#​4741]: tokio-rs/tokio#4741 [#​4755]: tokio-rs/tokio#4755 [#​4758]: tokio-rs/tokio#4758 [#​4762]: tokio-rs/tokio#4762 [#​4764]: tokio-rs/tokio#4764 [#​4766]: tokio-rs/tokio#4766 [#​4767]: tokio-rs/tokio#4767 [#​4769]: tokio-rs/tokio#4769 [#​4770]: tokio-rs/tokio#4770 [#​4772]: tokio-rs/tokio#4772 [#​4777]: tokio-rs/tokio#4777 [#​4791]: tokio-rs/tokio#4791 [#​4793]: tokio-rs/tokio#4793 [#​4795]: tokio-rs/tokio#4795 [#​4798]: tokio-rs/tokio#4798 [#​4806]: tokio-rs/tokio#4806 [#​4808]: tokio-rs/tokio#4808 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTEuMSIsInVwZGF0ZWRJblZlciI6IjMyLjExMS4xIn0=--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1458 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Extend
sync::watch::Receiver
to cover another use case.Motivation
As a
sync::watch::Receiver
I would like to know, if the value obtained fromborrow_and_update()
has actually changed and marked as seen or not. Subsequent calculations could be skipped without actually inspecting the value if no changes occurred.Solution
Add an
updated
has_changed
flag and method toRef
.Discarded solution (informational, just for the record )
Add a new function
borrow_and_update_if_changed()
that returns a tuple, both the reference and a flag indicating if the value has been detected as changed and marked as seen, i.e. updated. Using a boolean flag is consistent with the return value ofhas_changed()
on success.This function could otherwise not be implemented in terms of the public API, because invoking
has_changed()
andborrow_and_update()
independently is not thread-safe.As temporary a workaround you could invokeDoesn't work ashas_changed()
twice, i.e. before and after invokingborrow_and_update()
. This will cause some false positives and is less efficient. This doesn't work correctly if theSender
has been dropped, because thenhas_changed()
always returns with an error.Receiver
is already mutably borrowed afterborrow_and_update()
.