fix: return RecvError if channel is closed when calling watch::Receiver::changed

[DSharifi]

Motivation

Solves #7281 . The specification of watch::Receiver::changed says,

"This method returns an error if and only if the Sender is dropped."

A regression test has been added, changed_errors_on_closed_channel, to verify that the proposed changes fixes the implementation of changed to conform to he specification.

Solution

The PR fixes the bug in changed by adding an early check in changed_impl on whether the channel is closed before checking if the value is changed.

[ADD-SP]

I'm considering which behavior should be chosen.

Apparently, we should align the behavior of both has_changed and changed. Otherwise, inconsistent behavior will confuse downstream users. Now, we have two options when there is no active Sender.

Don't return RecvError until the latest value is marked as seen

This means:

• has_changed keeps returning Ok(true) until other methods (like changed or mark_changed) are called. After that, subsequent calls return RecvError.
• The first call to changed returns Ok(()), and subsequent calls return RecvError.

In this design, when there is no active sender, the downstream does not lose the last value. However, unless the downstream manually marks the value as seen (via changed or mark_changed), it cannot detect that the sender is no longer active.

Return RecvError regardless of whether a new value exists

This means:

• has_changed keeps returning RecvError even when a new value is available.
• changed also keeps returning RecvError even when a new value is available.

In this design, when there is no active sender, the downstream may lose the last value. However, it can immediately detect that there is no active sender.

---

I prefer the first option because:

1. It is unlikely for downstream to keep calling has_changed and no other code paths mark the value as seen.
2. My personal guess is that some downstream might be relying on the behavior that changed does not lose its last value.

Could you share your thoughts?

[DSharifi]

Thanks @ADD-SP for posting your thoughts!

Indeed, we should definitely align the behavior of both methods changed and has_changed. I don't have a strong opinion in what direction we go with to achieve that behavior, but here are my thoughts:

1. Don't return RecvError until the latest value is marked as seen

The use cases I've usually encountered where a watcher is used, the actor that receives the receiver handle is only alive as long as the channel and producer half is alive, meaning it's ideal to receive an Err(RecvError) IFF the channel is closed. For that reason I'd prefer this option.

Given that this change is a bug fix to match documented behavior I believe option 1 is a safe path.

2. Return RecvError regardless of whether a new value exists

For use cases where it is critical that the last value is preserved before shutting down, I'd expect such implementations to call borrow and process the last value before shutting down. Thus I think it's fine if we do don't return RecvError whether a new value exists or not.

However, like you mention option 2 is less likely to break any dependents if they somehow depend on the current behavior.

---

Sidenote

While we are on the topic of unifying the behavior, I find wait_for's behavior also a bit odd, and its documentation makes me no wiser.

From the docs:

If the channel is closed, then wait_for will return a RecvError. Once this happens, no more messages can ever be sent on the channel. When an error is returned, it is guaranteed that the closure has been called on the last value, and that it returned false for that value. (If the closure returned true, then the last value would have been returned instead of the error.)

The first sentence and the last sentence in parentheses seem to conflict each other. When testing the behavior I found that wait_for does indeed never return a RecvError if the predicate returns true, regardless of whether the value is seen or not.

[ADD-SP]

🫠 I can't understand and distinguish these interfaces in a few minutes.

[DSharifi]

Closing this PR.

Thinking more over it, your suggestion of instead changing the behavior of changed to not return RecvError for unseen values makes more sense.

Will reopen a new PR for it shortly.