sync: clarify the behavior of tokio::sync::watch::Receiver

[DSharifi]

Motivation

Closes #7281 and supersedes #7556.

changed has a documentation bug: its docs claimed it would error if and only if the channel was closed, but the implementation only returns an error when the channel is closed AND there are no unseen values.

Solution

• The documentation on fallibility of changed has been updated to reflect its implementation.
• has_changed's documentation has been updated to better highlight its difference with changed, as these methods fail on different conditions.
• The watch module documentation has been updated to reflect the difference between changed and has_changed.
• Integration tests have been added to test the intended behavior of changed and has_changed for closed channels.

[ADD-SP]

It looks good to me. I'll wait a few days before merging, in case others are concerned about the behavior change.

[Darksonn]

Honestly, I'm somewhat concerned about this. Yes, the watch channel is kind of broken, but it's also used widely and I think lots of people out there rely on all sorts quirks of its behavior. I'm wary of changing it.

[ADD-SP]

@Darksonn 's concern is valid. At least, we can just fix the incorrect document of changed.

[DSharifi]

Thanks for the feedback @Darksonn. I understand the concern about breaking changes if downstream users depend on quirks of the watch API.

I'm on the fence if it's a change that is likely of breaking downstream users:

1. has_changed erroring on unseen values while changed doesn't creates an inconsistent API that might cause confusion and bugs.
2. For a user to break on the change of has_changed erroring for unseen values, they would need to be calling has_changed repeatedly without ever calling methods that mark values as seen. Effectively using has_changed as a sync check to see if the channel is closed without ever marking values as seen. I do see that we don't have any sync method, for example closed, to explicitly check for channel closures synchronously.

If we're still concerned about breaking the above use case, I'm happy to revise the PR and only keep the documentation fix and regression tests.

[ADD-SP]

This is precisely why I'm so cautious about adding new features to tokio crate, the bar of changing/fixing the behavior of tokio is significantly higher than tokio-util.

[ADD-SP]

Given that behavioral changes in tokio is always sensitive, I recommend the following approach. I prioritize backward compatibility and documentation clarity, even if it deviates from ideal principles of interface design.

• The existing functionality of has_changed should remain unchanged.
• The document of changed should be fixed to reflect the existing behavior.
• Highlight different behavior between the has_changed and changed.

Here are my rationales

• The has_changed currently works as documented. More importantly, downstream may already rely on this specific behavior to detect the closed sender. While this is not a reasonable use case from my perspective, no dedicated method is available even for now, so changing this behavior now would likely introduce breaking changes for them.
• The current documentation makes it difficult to quickly distinguish between these two methods. We should high light it to help downstream use these interfaces correctly.

[ADD-SP]

Hi @DSharifi , are you still working on this PR?

[DSharifi]

Hi @DSharifi , are you still working on this PR?

Hey @ADD-SP, been a bit busy last week.

I'll update the PR tonight by reverting the behavioral changes, and instead update the documentation like you and @Darksonn suggested in the comments.

[ADD-SP]

Thanks!