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

watch::Sender: Modify and send value conditionally #4591

Merged
merged 6 commits into from
May 3, 2022

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Mar 31, 2022

TODO

  • Improve documentation

Often times you need to check preconditions and constraints before actually modifying a value. Currently receivers are notified unconditionally, even if the value has not been modified.

This PR adds a function send_if_modified() that is more versatile than send_modify(). The result of the passed closure indicates if the mutably borrowed value has actually been modified or not. Receivers are only notified if the value has been modified as indicated by the sender.

Signed-off-by: Uwe Klotz uwe.klotz@slowtec.de

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Mar 31, 2022
@uklotzde uklotzde force-pushed the watch_send_modified branch 4 times, most recently from acd4cdc to 1f3b5a3 Compare March 31, 2022 11:14
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Mar 31, 2022
@uklotzde
Copy link
Contributor Author

Getting the doctest right was trickier than expected. But now it is more restrictive and also considers the changed flag.

@Darksonn
Copy link
Contributor

A breaking change in the main Tokio crate is not going to happen.

@uklotzde
Copy link
Contributor Author

uklotzde commented Mar 31, 2022

What do you suggest? Add a new function send_modified() or send_modify_if()?

@Darksonn
Copy link
Contributor

A new function would be possible.

Add a function that is more versatile than send_modify(). The result of
the passed closure indicates if the mutably borrowed value has actually
been modified or not. Receivers are only notified if the value has been
modified as indicated by the sender.

Signed-off-by: Uwe Klotz <uwe.klotz@slowtec.de>
@uklotzde uklotzde changed the title watch::Sender: Modify value conditionally watch::Sender: Modify and send value conditionally Mar 31, 2022
@uklotzde
Copy link
Contributor Author

I have added a new function, force pushed, and updated the PR description.

The name of the new function and its parameter have been chosen to be readable and to reflect the expected behavior. Other suggestions are always welcome.

@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2022

Someone pointed out that that can already be implemented using watch::Sender::borrow to view the value before sending. Does this not match your use-case?

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 3, 2022

Someone pointed out that that can already be implemented using watch::Sender::borrow to view the value before sending. Does this not match your use-case?

How could this work reliably? Upgrading a read lock to a write lock could cause deadlocks. Therefore we need to acquire a write lock right from the start without marking the value as dirty.

@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2022

The watch channel is single-producer. With only one sender, there's no race even if you unlock the read lock to take the write lock.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 3, 2022

The watch channel is single-producer. With only one sender, there's no race even if you unlock the read lock to take the write lock.

Doesn't work for my FRP use case where I am sharing the Sender between different independent, async tasks by wrapping it into an Arc. Syntactically this is valid. But with this additional indirection on the sender side the only-notify-if-modfied pattern cannot be implemented safely, because different senders could otherwise deadlock each other.

Or do I need to use a full featured MPMC queue for this purpose?

@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2022

I suppose it is true that you can have multiple senders by wrapping it in an Arc.

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.

Alright, I think this change looks good. It's annoying that we've ended up with so many different send methods, but I suppose it is fine. I do have one comment regarding the docs though.

Comment on lines 590 to 593
/// The closure that modifies the value must return `true` if the
/// value has actually been modified. It should only return `false`
/// if the value is guaranteed to be unnmodified despite the mutable
/// borrow. Receivers are only notified if the value has been modified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Although not modifying it when you return false is the intended use-case, I still think we should document what happens if you modify the value and then return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While we are at it: Should we remove the redundancy in the docs and refer to the more versatile send_if_modified() in the docs for send_modify()? The docs would then basically reflect the forwarding of the implementation.

Copy link
Contributor

@Darksonn Darksonn Apr 7, 2022

Choose a reason for hiding this comment

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

Any improvements to the docs you can come up with would be good! Let me know when you think the docs are ready.

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 have revised and aligned the docs of both functions, highlighting their differences and similarities. Since I am not a native speaker someone should verify.

Removing the redundancies would be difficult. Copying the shared parts should be ok to keep the descriptions readable.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 6, 2022

Alright, I think this change looks good. It's annoying that we've ended up with so many different send methods, but I suppose it is fine. I do have one comment regarding the docs though.

Hopefully, this is now the most universal variant. Envisioning all possible use cases right from the start is always difficult.

Capturing intermediate results is possible by storing them in the (mutable) environment of the closure. Therefore I am confident that the boolean result extension is sufficient here.

Aligning the names by renaming it to send_modify_if() could be an option to reflect that the new function is an extension of the existing one. On the other hand send_if_modified() is more fluent to read and better reflects the purpose.

@uklotzde uklotzde marked this pull request as draft April 7, 2022 12:50
@uklotzde uklotzde marked this pull request as ready for review April 7, 2022 13:54
@Darksonn Darksonn enabled auto-merge (squash) May 3, 2022 06:53
@Darksonn Darksonn merged commit c14566e into tokio-rs:master May 3, 2022
@uklotzde uklotzde deleted the watch_send_modified branch May 3, 2022 09:11
uklotzde added a commit to uklotzde/tokio that referenced this pull request May 8, 2022
Add a function that is more versatile than send_modify(). The result of
the passed closure indicates if the mutably borrowed value has actually
been modified or not. Receivers are only notified if the value has been
modified as indicated by the sender.

Signed-off-by: Uwe Klotz <uwe.klotz@slowtec.de>
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 11, 2022
This PR contains the following updates:

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

---

### Release Notes

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

### [`v1.19.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.1)

[Compare Source](tokio-rs/tokio@tokio-1.19.0...tokio-1.19.1)

##### 1.19.1 (June 5, 2022)

This release fixes a bug in `Notified::enable`. ([#&#8203;4747])

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

### [`v1.19.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.0)

[Compare Source](tokio-rs/tokio@tokio-1.18.2...tokio-1.19.0)

##### 1.19.0 (June 3, 2022)

##### Added

-   runtime: add `is_finished` method for `JoinHandle` and `AbortHandle` ([#&#8203;4709])
-   runtime: make global queue and event polling intervals configurable ([#&#8203;4671])
-   sync: add `Notified::enable` ([#&#8203;4705])
-   sync: add `watch::Sender::send_if_modified` ([#&#8203;4591])
-   sync: add resubscribe method to broadcast::Receiver ([#&#8203;4607])
-   net: add `take_error` to `TcpSocket` and `TcpStream` ([#&#8203;4739])

##### Changed

-   io: refactor out usage of Weak in the io handle ([#&#8203;4656])

##### Fixed

-   macros: avoid starvation in `join!` and `try_join!` ([#&#8203;4624])

##### Documented

-   runtime: clarify semantics of tasks outliving `block_on` ([#&#8203;4729])
-   time: fix example for `MissedTickBehavior::Burst` ([#&#8203;4713])

##### Unstable

-   metrics: correctly update atomics in `IoDriverMetrics` ([#&#8203;4725])
-   metrics: fix compilation with unstable, process, and rt, but without net ([#&#8203;4682])
-   task: add `#[track_caller]` to `JoinSet`/`JoinMap` ([#&#8203;4697])
-   task: add `Builder::{spawn_on, spawn_local_on, spawn_blocking_on}` ([#&#8203;4683])
-   task: add `consume_budget` for cooperative scheduling ([#&#8203;4498])
-   task: add `join_set::Builder` for configuring `JoinSet` tasks ([#&#8203;4687])
-   task: update return value of `JoinSet::join_one` ([#&#8203;4726])

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

</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.

---

 - [x] <!-- 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).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1394
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>
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 Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants