Client and server timeout support

[cpu]

This takes the client-side timeout work from #178 with some review feedback applied, and extends it to the server-side (both for the simple case, as well as the lazy acceptor machinery).

The review feedback modifications to #178 can be reviewed as unsquashed fixup commits on this branch. In summary:

• switched an expect() to an unreachable!() with some commentary (a6d5566)
• In-lined a single-use helper (5e7d2cf)
• Re-ordered some elements (997ad7c)
• Reworked the original unit test to use tokio's start_paused (4a0b85d)
• Added a bit more coverage (90d54c6)
• Extracted the new code somewhere I could reuse it (b966f10)

Afterwards I overlaid server support for TlsAcceptor/FallibleAccept, and then extended it for the LazyConfigAcceptor and into_stream() bits.

[cpu]

Lastly, I bumped the aws-lc-rs version to fix a windows CI builder failure I was seeing otherwise.

Dropped this. I think I just had a stale base branch.

[djc]

Why couldn't we use a tokio::time::Timeout<MindHandshake<IS>> instead of defining this wrapper ourselves?

[cpu]

I tried that initially after reviewing 178 and having the same thought, but it didn't seem like a good fit to me after I tried it a bit.

• We still end up needing a wrapper enum or similar indirection for whether there is/isn't a Timeout unless we do something hacky like always allocating a Timeout with max duration.
• Timeout is !Unpin but MidHandshake and everything else is currently Unpin, so we need to Pin<Box<...>> it at the cost of a heap allocation and more nesting complexity.
• Most importantly, I couldn't figure out a way to maintain Connect::get_mut() with a Pin<Box<Timeout<...>>> or the FallibleConnect implementation where we need to recover the IO without using unsafe.

I'm not particularly adept at async rust trickery, but achieving this in a semver compatible way using tokio::time::Timeout without any new deps or unsafe didn't seem tractable, and the implementation as done in this branch (and originally in 178) doesn't seem particularly complex in comparison.

If you think I'm missing something maybe we could land this approach and then refactor it down to a tokio::time::Timeout as follow-up?

[djc]

Thanks for picking this up!

[thalesfragoso]

I'm still not sure if this feature belongs in the crate, you don't see something like this in e.g. tokio::net::TcpStream, specifically because canceling futures is "trivial" in rust. Arguably, the only thing that can't be implemented outside of the crate is the take_io method.

Moreover, this implementation is not even optimal because we have to box the Sleep future or use unsafe. We can't even use e.g. pin-project-lite because it would make the Connect future !Unpin and that would be a breaking change.

[thalesfragoso]

I guess my suggestion is to just implement take_io in the Connect future.

[cpu]

Thanks for taking a look.

I'm still not sure if this feature belongs in the crate, you don't see something like this in e.g. tokio::net::TcpStream, specifically because canceling futures is "trivial" in rust.

Perhaps, but I'm not sure I'm completely convinced of that yet. I think the linked issues show there's some demand and having each consumer implementing the same things themselves if we can offer it sensibly once via this crate might not be optimal.

Arguably, the only thing that can't be implemented outside of the crate is the take_io method.

I agree on the connect side that there are approaches to doing most of this outside the crate but I do think the take_io() part is important, and also don't know that this line of thinking extends as clearly to the server-side portions.

Moreover, this implementation is not even optimal because we have to box the Sleep future or use unsafe. We can't even use e.g. pin-project-lite because it would make the Connect future !Unpin and that would be a breaking change.

I'm not sure this feedback applies to the code in-branch? It isn't using pin-project, or unsafe, or making any breaking changes.

I guess my suggestion is to just implement take_io in the Connect future.

If you want to open an alternative PR that achieves equivalent support for timeouts across both client/server side internally in a different way, or shows how to achieve it comprehensively from examples/ with less crate changes I'd be open to reviewing.

[cpu]

I'm still not sure if this feature belongs in the crate,

Opinions can of course change over time, but part of why I looked at this was an initial signal from Djc in discord that this crate would be an acceptable place to make a change like this.

[thalesfragoso]

I'm not sure this feedback applies to the code in-branch? It isn't using pin-project, or unsafe, or making any breaking changes.

I meant that the code in-branch is not (technically) optimal because it boxes the Sleep future only to avoid breaking changes.

If you want to open an alternative PR that achieves equivalent support for timeouts across both client/server side internally in a different way...

My questioning is exactly if we should implement this internally or not. Your implementation is perfectly fine given the constraints, and I'm not strongly opposed to merging this PR.