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.

Sign up for GitHub

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

Jump to bottom

sync: implement watch::Receiver::wait_for method #5611

Merged
merged 33 commits into from
Apr 24, 2023
Merged

sync: implement watch::Receiver::wait_for method #5611

merged 33 commits into from
Apr 24, 2023

Conversation

debadree25
Copy link
Contributor

Hey! attempting my first contribution here! confused about whether tests would be required here!

Thank You!

Fixes: #5606

Motivation

The motivation for this API is that in the case of using the changed() API it is required that the receiver first check for changes and then wait for further changes failing to do so would result in race conditions.

Solution

We have solved the stated issue with changed() by adding a watch_for API which takes in a closure and continuously passes the present value on changes to the closure and returns once the closure returns true

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Apr 9, 2023
@debadree25
Copy link
Contributor Author

I think failures seem to be CI flakes?

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Apr 10, 2023
@Darksonn
Copy link
Contributor

Merge in master to fix CI.

Darksonn
Darksonn reviewed Apr 11, 2023
View reviewed changes
tokio/src/sync/tests/loom_watch.rs Outdated Show resolved Hide resolved
tokio/src/sync/tests/loom_watch.rs Outdated Show resolved Hide resolved
@debadree25 debadree25 requested a review from Darksonn April 11, 2023 10:07
Darksonn
Darksonn reviewed Apr 11, 2023
View reviewed changes
tokio/src/sync/tests/loom_watch.rs Outdated Show resolved Hide resolved
tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
@debadree25 debadree25 requested a review from Darksonn April 12, 2023 04:53
Darksonn
Darksonn reviewed Apr 12, 2023
View reviewed changes
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.

I still think the documentation could be improved, but the implementation and test looks reasonable enough.

tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
@debadree25
Copy link
Contributor Author

I still think the documentation could be improved, but the implementation and test looks reasonable enough

Any specific things you don't like about it? can try to rewrite along those lines

Darksonn
Darksonn reviewed Apr 15, 2023
View reviewed changes
tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
nicolasmendoza
nicolasmendoza reviewed Apr 19, 2023
View reviewed changes
Copy link

@nicolasmendoza nicolasmendoza left a comment

Choose a reason for hiding this comment

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

LGTM, just some comments and questions.

tokio/src/sync/watch.rs Show resolved Hide resolved
tokio/src/sync/tests/loom_watch.rs Show resolved Hide resolved
tokio/src/sync/tests/loom_watch.rs Show resolved Hide resolved
@debadree25
Copy link
Contributor Author

Hey @Darksonn i updated the code to include version matching as we discussed PTAL

Thank You!

@debadree25 debadree25 requested a review from Darksonn April 22, 2023 07:39
Darksonn
Darksonn reviewed Apr 22, 2023
View reviewed changes
tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
tokio/src/sync/watch.rs Outdated Show resolved Hide resolved
@debadree25
Copy link
Contributor Author

Fixed the linting and clippy errors!

Darksonn
Darksonn reviewed Apr 23, 2023
View reviewed changes
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.

I think this looks good. I have a few test-specific comments. Other than testing, I think this is ready. 👍

tokio/src/sync/tests/loom_watch.rs Outdated Show resolved Hide resolved
tokio/src/sync/tests/loom_watch.rs Show resolved Hide resolved
Darksonn
Darksonn approved these changes Apr 24, 2023
View reviewed changes
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. This LGTM.

@Darksonn Darksonn merged commit c1778ed into tokio-rs:master Apr 24, 2023
@debadree25
Copy link
Contributor Author

Thank you so much @Darksonn for all your help! I learnt a lot and look forward to contributing more! 😄😄

@Darksonn Darksonn mentioned this pull request Apr 25, 2023
Merged
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Apr 29, 2023
Update Rust crate tokio to 1.28.0 (#1875)
b35dea6 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.27.0` -> `1.28.0` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.27.0` -> `1.28.0` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.28.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.28.0): Tokio v1.28.0

[Compare Source](tokio-rs/tokio@tokio-1.27.0...tokio-1.28.0)

##### 1.28.0 (April 25th, 2023)

##### Added

-   io: add `AsyncFd::async_io` ([#&#8203;5542])
-   io: impl BufMut for ReadBuf ([#&#8203;5590])
-   net: add `recv_buf` for `UdpSocket` and `UnixDatagram` ([#&#8203;5583])
-   sync: add `OwnedSemaphorePermit::semaphore` ([#&#8203;5618])
-   sync: add `same_channel` to broadcast channel ([#&#8203;5607])
-   sync: add `watch::Receiver::wait_for` ([#&#8203;5611])
-   task: add `JoinSet::spawn_blocking` and `JoinSet::spawn_blocking_on` ([#&#8203;5612])

##### Changed

-   deps: update windows-sys to 0.48 ([#&#8203;5591])
-   io: make `read_to_end` not grow unnecessarily ([#&#8203;5610])
-   macros: make entrypoints more efficient ([#&#8203;5621])
-   sync: improve Debug impl for `RwLock` ([#&#8203;5647])
-   sync: reduce contention in `Notify` ([#&#8203;5503])

##### Fixed

-   net: support `get_peer_cred` on AIX ([#&#8203;5065])
-   sync: avoid deadlocks in `broadcast` with custom wakers ([#&#8203;5578])

##### Documented

-   sync: fix typo in `Semaphore::MAX_PERMITS` ([#&#8203;5645])
-   sync: fix typo in `tokio::sync::watch::Sender` docs ([#&#8203;5587])

[#&#8203;5065]: tokio-rs/tokio#5065

[#&#8203;5503]: tokio-rs/tokio#5503

[#&#8203;5542]: tokio-rs/tokio#5542

[#&#8203;5578]: tokio-rs/tokio#5578

[#&#8203;5583]: tokio-rs/tokio#5583

[#&#8203;5587]: tokio-rs/tokio#5587

[#&#8203;5590]: tokio-rs/tokio#5590

[#&#8203;5591]: tokio-rs/tokio#5591

[#&#8203;5607]: tokio-rs/tokio#5607

[#&#8203;5610]: tokio-rs/tokio#5610

[#&#8203;5611]: tokio-rs/tokio#5611

[#&#8203;5612]: tokio-rs/tokio#5612

[#&#8203;5618]: tokio-rs/tokio#5618

[#&#8203;5621]: tokio-rs/tokio#5621

[#&#8203;5645]: tokio-rs/tokio#5645

[#&#8203;5647]: tokio-rs/tokio#5647

</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, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42My4xIiwidXBkYXRlZEluVmVyIjoiMzUuNjQuMCJ9-->

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1875
Reviewed-by: crapStone <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasmendoza nicolasmendoza nicolasmendoza left review comments

@Darksonn Darksonn Darksonn approved these changes

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync R-loom Run loom tests on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sync: add watch::Receiver::wait_for
3 participants
@debadree25 @Darksonn @nicolasmendoza