Skip to content

watch::Sender::send is a error-prone default #7228

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

Open
dpc opened this issue Mar 20, 2025 · 2 comments
Open

watch::Sender::send is a error-prone default #7228

dpc opened this issue Mar 20, 2025 · 2 comments
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-sync Module: tokio/sync

Comments

@dpc
Copy link
Contributor

dpc commented Mar 20, 2025

Description

We've recently been banning watch::Sender:send method in our codebases because:

However, when send fails, the value isn’t made available for future receivers (but returned with the SendError).

is a non-obvious behavior that leads to tricky to debug bugs.

From my PoV it's very natural to use a watch channel as a "latest value with an ability to wait for updates", andsend name does not immediately point to the trickiness of behavior of "dropping updates" in it. Yes, the returned result should nudge the user to "why is there a possibility of an error, and what happens in this situation", but in practice it has been missed multiple times, and it's quite common in other channels to do let _ = tx.send(); because dropping messages when no one listens doesn't matter for them because how they are used they don't have state that would matter for subscribes that join later. And when one looks at tx they can't immediately tell what channel is it, so they simply won't notice this is a watch::Sender and dropped message might actually matter a lot.

send_replace's behavior seems much more safer default, and therefore (IMO) better candidate for a default (shortest name). But I could accept someone disagreeing with. Possibly there should not be a send at all, and current send should be renamed to send_xyz (send_and_update_if_connected?) to bring to the user's attention that they should consider alternatives and details between them.

@dpc dpc added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Mar 20, 2025
dpc added a commit to dpc/fedimint that referenced this issue Mar 20, 2025
Require `#[allow(...)]` if it's semantics are actually needed

See motivation in: tokio-rs/tokio#7228
dpc added a commit to dpc/fedimint that referenced this issue Mar 20, 2025
Require `#[allow(...)]` if it's semantics are actually needed

See motivation in: tokio-rs/tokio#7228
@Darksonn Darksonn added the M-sync Module: tokio/sync label Mar 21, 2025
@Darksonn
Copy link
Contributor

I'm not entirely happy with the watch channel, but we can't remove or rename methods due to backwards compatibility.

@maan2003
Copy link

but we can still mark it as depreciated, IMO it is bad enough to deserve that.

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 C-bug Category: This is a bug. M-sync Module: tokio/sync
Projects
None yet
Development

No branches or pull requests

3 participants