futures::channel::oneshot severe bug affecting cleanup
futures::channel::oneshot::Receiver has a broken implementation of FusedFuture which makes it not work in select!. If the Sender is dropped, the select! functions as if the Receiver is never Ready (even though when you poll it it is Ready in this case). https://github.com/rust-lang/futures-rs/issues/2455
One big problem with this that on shutdown of part of Arti, it can cause tasks to hang rather than noticing everything has gone away and terminating.
Options:
-
Do something (what?) to try to spot when a
oneshot::Receiveris used withinselect!, and call.fuse()on it (and store that somewhere) to get a properly working impl ofFusedFuture. I don't think this is very viable unless we have a more concrete idea. (Hrm, can we make<Receiver as FusedFuture>::is_terminateda deprecated method and spot its uses byselect!?) -
Use Tokio's
oneshot. Inspection of the implementation (thanks @trinity-1686a) reveals that it doesn't depend on the Tokio executor. Downside: every crate now ends up with a dep on Tokio. This is bad because it would make it easy to use other bits of tokio, despite our intent to be runtime-agnostic. It's also bad for our code size. -
Use Tokio's
oneshotbut via a re-export it fromtor-async-utils.That helps prevent the accidental use of Tokio but it doesn't help with code size. -
Copy Tokio's
oneshot.rsintotor-async-utils. -
Use
postage::oneshot. But it has a different (and rather less good) API. -
Use
postage::oneshotbut wrap it in a newtype. I'm not sure the implementation of the good API in terms of the less good one will be much fun or very principled. -
Write our own
oneshot. -
Use
FusedFuture<oneshot::Receiver<T>>everywhere and enforce this by (i) making all the constructors likeoneshot::channeldeprecated and (ii) providing wrappers that wrap theReceiver. (This is an extra byte (probably word) of state for eachReceiverand therefore will have a perf impact, so on super hot paths we might want something else. But we don't have many oneshots on super hot paths.)
CC @gabi-250 @nickm since this'll probably end up being a widespread thing.