util: implement ReusableBoxFuture with one function

[SabrinaJewson]

Motivation

ReusableBoxFuture is currently complex-ish, with unsafe code and types in multiple places.

Solution

Move all the unsafety into a single function reuse_pin_box (and a helper type). I think this makes the definition of ReusableBoxFuture a bit simpler, and I'm pretty sure that it's sound. The function has the signature:

fn reuse_pin_box<T: ?Sized, U, O, F>(boxed: Pin<Box<T>>, new_value: U, callback: F) -> Result<O, U>
where
    F: FnOnce(Box<U>) -> O,

It encapsulates the basic premise of ReusableBoxFuture — a Box that can be reused — and ReusableBoxFuture is then implemented on top of it. It accepts a callback instead of returning Result<Box<U>, O> so that code can be run even if dropping the old T fails.

[SabrinaJewson]

By the way, I didn't catch that Clippy warning initially because Tokio doesn't even build for my with my local Clippy. If I try it just produces this error:

Click to show Clippy output

warning: this expression borrows a value the compiler would automatically borrow
  --> tokio/src/future/poll_fn.rs:38:9
   |
38 |         (&mut self.f)(cx)
   |         ^^^^^^^^^^^^^ help: change this to: `self.f`
   |
   = note: `#[warn(clippy::needless_borrow)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: methods called `is_*` usually take `self` by reference or no `self`
  --> tokio/src/io/util/vec_with_initialized.rs:67:28
   |
67 |     pub(crate) fn is_empty(&mut self) -> bool {
   |                            ^^^^^^^^^
   |
   = note: `#[warn(clippy::wrong_self_convention)]` on by default
   = help: consider choosing a less ambiguous name
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention

warning: unnecessary use of `to_vec`
   --> tokio/src/net/addr.rs:148:20
    |
148 |         let iter = self.to_vec().into_iter();
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `self.iter().copied()`
    |
    = note: `#[warn(clippy::unnecessary_to_owned)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned

warning: `tokio` (lib) generated 3 warnings
    Checking tokio v1.18.2 (/home/sabrina/code/tokio/tokio)
    Checking tokio-stream v0.1.8 (/home/sabrina/code/tokio/tokio-stream)
error: written amount is not handled
  --> tokio/tests/tcp_split.rs:39:5
   |
39 |     write_half.write(MSG).await?;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[deny(clippy::unused_io_amount)]` on by default
   = help: use `AsyncWriteExt::write_all` instead, or handle partial writes
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unused_io_amount

Not sure what's going on here but it would be nice if that could be fixed

[Darksonn]

Tokio uses an older clippy version because some of the new lints are incompatible with our minimum supported Rust version. You can find the version we use in the CI configuration.

[hawkw]

Overall, this approach looks good to me --- having the ReusableBoxFuture own a Pin<Box<...>> definitely feels much nicer than the NonNull, as we need less unsafe code outside of the code that actually reuses the allocation.

I commented on a few relatively minor things.

[hawkw]

this appears to be the only place where reuse_pin_box is ever called...do you think it would make sense to just put that code in this function? that way we avoid having an additional function that gets monomorphized over a big pile of generic types, which might help with binary size a bit...

[SabrinaJewson]

Conceptually I quite like the idea of reuse_pin_box as an independent futures-unaware utility function — it makes a lot of sense to me as a layer of abstraction. Would putting #[inline(always)] on the function have the same effect?

[Darksonn]

My guess is that it'll be inlined either way.

[hawkw]

Yeah, I think it'll get inlined either way.

I'm not strongly opposed to keeping the separate function; I just wanted to note that it isn't currently used elsewhere. 🤷‍♀️

[Darksonn]

Thanks.