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: add Receiver::borrow_and_update #3813

Merged
merged 2 commits into from
Jun 16, 2021
Merged

sync: add Receiver::borrow_and_update #3813

merged 2 commits into from
Jun 16, 2021

Conversation

Darksonn
Copy link
Contributor

Add a method similar to borrow that updates the version counter. Using this method, it would be possible to update WatchStream to avoid the issue described by #3655.

Fixes: #3666
Closes: #3731

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels May 26, 2021
@Darksonn Darksonn requested a review from carllerche May 26, 2021 08:14
@carllerche
Copy link
Member

My guess is you considered updating the borrow() fn to this behavior before adding a new fn and noticed that the version field in Receiver couldn't be updated with &self? Looking at the code, I would probably change Receiver::version to an AtomicUsize. It should be safe to load / store w/ SeqCst while the read lock is held. Shared::version can only be updated when the write lock is held, so any concurrent calls to Receiver::borrow will observe the same value.

@carllerche
Copy link
Member

Looking more at this, I don't see any obvious reason for Shared::version to be atomic instead of just being guarded by the RwLock. Switching to that would probably simplify the code.

@Darksonn
Copy link
Contributor Author

My guess is you considered updating the borrow() fn to this behavior before adding a new fn and noticed that the version field in Receiver couldn't be updated with &self?

No, that isn't the reason. It seems to me that changing the behavior of borrow() in this manner should constitute a breaking change.

@Darksonn
Copy link
Contributor Author

Looking more at this, I don't see any obvious reason for Shared::version to be atomic instead of just being guarded by the RwLock. Switching to that would probably simplify the code.

As it is now, the changed method doesn't take any locks, which would have to change if we make it non-atomic. That said, we might be able to relax the ordering.

Besides that, there's also the closed flag which is stored as the least-significant bit in the version counter.

@carllerche
Copy link
Member

Hmmm, yes, technically it could be a breaking change as we documented changed() as notifying if there were any changes since last call to changed(). I think it is debatable whether it is really a breaking change, but I'm ok w/ adding a new fn.

We probably should be sure to document the differences between the two borrow_* fns (each fn can include a bit saying how it is different from the other).

We probably should tweak the changed() doc as it won't raise a notification if a change occurred since the last call to changed() if borrow_and_update was called.

@carllerche
Copy link
Member

Looking more at this, I don't see any obvious reason for Shared::version to be atomic instead of just being guarded by the RwLock. Switching to that would probably simplify the code.

As it is now, the changed method doesn't take any locks, which would have to change if we make it non-atomic. That said, we might be able to relax the ordering.

Besides that, there's also the closed flag which is stored as the least-significant bit in the version counter.

True... though fetch_add could be changed to load / store i think... anyway, it isn't related to this PR, so no need to make that change.

@Darksonn
Copy link
Contributor Author

Darksonn commented Jun 2, 2021

What do you think of the name of the function?

@carllerche
Copy link
Member

It seems fine!

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks 👍

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make possible to update version of watch to version in shared data
2 participants