sync: oneshot::Receiver::{is_empty, is_closed}

[cratelyn]

this commit proposes two new methods for tokio's
sync::oneshot::Receiver<T> type: is_empty() and is_closed().

these follow the same general pattern as existing is_empty() and is_closed() methods on other channels such as
sync::mpsc::Sender::is_closed(), sync::mpsc::Receiver::is_closed(), sync::oneshot::Receiver::is_closed(), sync::broadcast::Receiver::is_empty(), sync::mpsc::Receiver::is_empty(), ...and so forth.

broadly speaking, users of the oneshot channel are encouraged to .await the Receiver<T> directly, as it will only yield a single value.

users that are writing low-level code, implementing Futures directly, may instead poll the receiver via
<Receiver<T> as Future<Output = Result<T, RecvError>::poll(..).

because (a) poll() has an &mut self receiver that does not take ownership in the same way as .awaiting the receiver will, and (b) the contract of std::future::Future states that clients should not poll a future after it has yielded Poll::Ready(value), users implementing Futures in terms of a oneshot channel do not have a way to inspect the state of a oneshot channel.

this commit aims to provide such users means to passively inspect the state of a oneshot channel, to avoid violating the contact of Future::poll(..), or requiring that a oneshot channel users track this state themselves externally.

[hawkw]

Hmm, this feels somewhat different than the semantics of is_closed on other channels, which only returns true when the other side has been dropped. On the other hand, there's a valid argument that this is just because those channels don't have the property that sending a value closes the channel. I'm not sure how I feel about this: I think it might be worth being consistent that "closed" means "dropped by the other side", but I'm open to being convinced otherwise.

[hawkw]

Also, the Sender::is_closed method currently exists and only inspects the value of the CLOSED bit:

tokio/tokio/src/sync/oneshot.rs

Lines 719 to 751 in 0931406

	/// Returns `true` if the associated [`Receiver`] handle has been dropped.
	///
	/// A [`Receiver`] is closed by either calling [`close`] explicitly or the
	/// [`Receiver`] value is dropped.
	///
	/// If `true` is returned, a call to `send` will always result in an error.
	///
	/// [`Receiver`]: Receiver
	/// [`close`]: Receiver::close
	///
	/// # Examples
	///
	/// ```
	/// use tokio::sync::oneshot;
	///
	/// #[tokio::main]
	/// async fn main() {
	/// let (tx, rx) = oneshot::channel();
	///
	/// assert!(!tx.is_closed());
	///
	/// drop(rx);
	///
	/// assert!(tx.is_closed());
	/// assert!(tx.send("never received").is_err());
	/// }
	/// ```
	pub fn is_closed(&self) -> bool {
	let inner = self.inner.as_ref().unwrap();
	let state = State::load(&inner.state, Acquire);
	state.is_closed()
	}

Of course, that method can't ever be called if a value was sent, as doing so consumes the sender. so. Hm.

[Darksonn]

Yeah ... I'm not sure what is the least surprising here.

[cratelyn]

this is a very interesting conundrum!

to begin by restating the problem directly: we are asking ourselves whether or
not tokio::sync::oneshot::Receiver::is_closed() should return true when
the corresponding tokio::sync::oneshot::Sender has been consumed by sending
a value.

let's consider the alternatives and how they would affect callers using them
to determine whether or not a receiver may be polled.

option a: is_closed() does not include completed channel

in this model, is_closed() only returns true when the sender is dropped, or
when the receiver closes the channel. it does not return true if a value
was sent.

in this case, we would see the following values when inquiring about closedness
and emptiness.

-	is_closed()	is_empty()	safe to poll?
initial	false	true	yes
receiver closes channel	true	true or false	yes
sender was dropped	true	true	yes
sender sent value (unreceived)	false	false	yes
sender sent value (received)	false	true	no

option b: is_closed() does include completed channel

in this model, is_closed() returns true when the receiver closes the channel,
when the sender is dropped, or when the sender is used to send a value.

-	is_closed()	is_empty()	safe to poll?
initial	false	true	yes
receiver closes channel	true	true or false	yes
sender was dropped	true	true	yes
sender sent value (unreceived)	true	false	yes
sender sent value (received)	true	true	no

⚖️ comparing a and b

now, this brings us to @hawkw's note:

Hmm, this feels somewhat different than the semantics of is_closed on other
channels, which only returns true when the other side has been dropped. On
the other hand, there's a valid argument that this is just because those
channels don't have the property that sending a value closes the channel. I'm
not sure how I feel about this: I think it might be worth being consistent
that "closed" means "dropped by the other side", but I'm open to being
convinced otherwise.

first, i would note that we actually have some minor semantic differences
between different channels' respective is_closed methods already, upon
closer inspection. i'd argue that is fine, and inherent to the fact that
different channel have different semantics, and that is bound to be reflected
in methods querying their conceptual state.

as an example, sync::watch::Sender::is_closed returns true if all receivers
are dropped, while sync::mpsc::Sender::is_closed returns true if all
receivers are dropped or when Receiver::close is called. a sync::watch
channel can have many receivers, so it makes sense that its notion of
closedness would be narrower than a channel that conversely has a single
receiver.

it seems like it would be in line with the above for a oneshot channel to
have its own unique caveat related for is_closed, returning true when the
sender has been dropped by virtue of having sent a value.

...a secret third thing?

now, zooming back out for a moment: i found myself curious about this because
i've frequently encountered a pattern when interacting with oneshot channels
wherein they are very frequently wrapped in an Option<T>. once the receiver
is polled and yields a Poll::Ready(_) value of some kind, the caller takes
the receiver out of the Option<T> and mem::drop()s it to prevent from being
polled again.

prior to reading the internals of the oneshot channel, and learning more
about how the sender and receiver interact, my initial intuition was that one
could check that a receiver was finished by checking that the channel was
closed, and empty.

looking at the tables above however, i've found that neither option a or b
provide a surefire way to check that a receiver is finished, or if it is safe
to poll(). for example: an empty, closed channel could indicate that the
sender was dropped, but that doesn't provide information about whether the
Err(RecvError) has been returned yet.

now, looking at the receiver, and its Future implementation:

// tokio/src/sync/oneshot.rs (abbrevated)

#[derive(Debug)]
pub struct Receiver<T> {
    inner: Option<Arc<Inner<T>>>,
    // spans...
}

impl<T> Future for Receiver<T> {
    type Output = Result<T, RecvError>;

    fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        // If `inner` is `None`, then `poll()` has already completed.
        let ret = if let Some(inner) = self.as_ref().get_ref().inner.as_ref() {
            ready!(inner.poll_recv(cx))?
        } else {
            panic!("called after complete");
        };

        self.inner = None;
        Ready(Ok(ret))
    }
}

i see utility for a third method tokio::sync::oneshot::Receiver::is_finished().

impl<T> Receiver<T> {
    /// Checks if this receiver is finished.
    ///
    /// This will return true after this receiver has been polled and yielded
    /// a result.
    fn is_finished(&self) -> bool {
        self.inner.is_none()
    }
}

if this was an option, i would feel personally much less attached to the
decision over whether or not is_closed reports true when a value has been
sent.

this would directly let callers check if polling the receiver would panic, and
in friendly, direct terms. this would also mean that the common pattern of
wrapping receivers in Option<Receiver<T>> would be largely outmoded.
is_finished would let us inquire about the state of the receiver's own
internal Option<T>, without needing to wrap it in another outer option we
can inspect.

what do you think, @Darksonn, and @hawkw?

[Darksonn]

If the intent of this PR is a way to detect whether polling the channel will panic, then having a function that does exactly that sounds good to me.

[cratelyn]

the intent of this PR, broadly speaking, is to introduce accessors that allows sync::oneshot::Receiver<T> to inspect the channel's status in the same manner that other channels like sync::mpsc's and sync::broadcast's respective Receiver<T>s can.

detecting panics is one acute example, but i've also felt a need for these interfaces when writing Body implementations for types backed by a oneshot, to provide another example.

a Body backed by a oneshot receiver cannot report proper is_end_stream() hints because the receiver doesn't provide a way to inspect the channel. these felt like relatively agnostic methods to expose in part because they follow patterns exposed by other channels.

91682a9 introduces an is_finished() method. i'd be happy to outline that into a separate PR if we don't feel a disposition to merge is_empty() and is_closed() methods. what do you think?

[hawkw]

91682a9 introduces an is_finished() method. i'd be happy to outline that into a separate PR if we don't feel a disposition to merge is_empty() and is_closed() methods. what do you think?

Personally, I'd be inclined to suggest a PR adding a method like that separately, so that the change you actually need isn't blocked on deciding the right semantics for is_closed. I think that change should hopefully be fairly uncontroversial.

Regarding naming for that method, I think "is_finished" is probably fine, but I'll note that the futures crate's FusedFuture trait calls its method with similar semantics is_terminated. Although we probably won't be adding FusedFuture implementations in Tokio due to stability concerns, it might be worth choosing a name that's consistent with the one in futures? I could be convinced either way though.

[hawkw]

And, to be clear, I'd also still like to have is_closed() and is_empty() — I just think we could add an is_finished_and_or_terminated() without having to wait for figuring out is_closed().

I think we could probably also land an is_empty() method separately, if you like. I don't think there's much ambiguity about what "empty" means in this case.

[cratelyn]

thank you both for the advice and for your time, @Darksonn and @hawkw! i agree, landing these each separately sounds like a more prudent path forward.

i'm going to close this, and let us consider each of these proposals individually.

see:

• sync: oneshot::Receiver::is_terminated() #7152
• sync: oneshot::Receiver::is_empty() #7153
• sync: oneshot::Receiver::is_closed() #7154

[cratelyn]

this work is continued in #7152, #7153, and #7154.