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: implement watch::Receiver::mark_unseen() #5962

Merged
merged 6 commits into from
Sep 11, 2023

Conversation

victor-timofei
Copy link
Contributor

Fixes: #5871

Solution

I've added an extra flag to sync::watch::Receiver.

Mutex is used for thread safety and Cell for interior mutability.

Could try to use an AtomicBool instead of Mutex<Cell<bool>> since a Receiver instance is not shared across threads?

@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Aug 29, 2023
@Darksonn
Copy link
Contributor

I'm pretty sure you can just decrement the version instead. Since whether it's seen is computed based on what the version is.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Aug 29, 2023
@victor-timofei
Copy link
Contributor Author

I was thinking to change it to isize to be able to mark as unread even a newly created Receiver.

I was a little worried about the fact that there is also the CLOSED bit, but after a little experimentation I should be able to make it work.

@victor-timofei victor-timofei changed the title sync: imlement watch::Receiver::mark_unseen() sync: implement watch::Receiver::mark_unseen() Aug 30, 2023
@victor-timofei victor-timofei marked this pull request as ready for review August 31, 2023 09:42
@Darksonn
Copy link
Contributor

Darksonn commented Sep 1, 2023

You can just make the starting version be 1. Then you can always subtract one from the version.

tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
Comment on lines +56 to +57
#[test]
fn rx_mark_unseen() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like a test that actually sends a value somewhere.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks.

@Darksonn Darksonn merged commit 61042b4 into tokio-rs:master Sep 11, 2023
@uklotzde
Copy link
Contributor

I didn't notice this PR and left some comments at the merge commit: 61042b4

@uklotzde
Copy link
Contributor

uklotzde commented Sep 17, 2023

Most crucial issue is the choice of naming which introduces "unseen" while we already have changed() and has_changed(). IMHO mark_unchanged() mark_changed() would be the more consistent choice here.

@victor-timofei
Copy link
Contributor Author

Hi @uklotzde, sounds reasonable. Since the change hasn't been released yet I could update the name if we want to.

@uklotzde
Copy link
Contributor

I have created a PR to prevent that this change is released accidentally: #6014

@uklotzde
Copy link
Contributor

Got it wrong 🙈 Of course the method must be named mark_changed(), not mark_unchanged()!

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 M-sync Module: tokio/sync R-loom-sync Run loom sync tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability for sync::watch::Receiver to flag current item as unseen
3 participants