refactor(exec): kill the H2Exec trait

[djc]

As briefly discussed on Discord. Hope that this is the right way to go about it. If it is, I'm happy to also kill off NewSvcExec.

[seanmonstar]

I was trying to find the discussion, but seemed to miss it. Could you highlight it (or link to the start of it)?

[djc]

I meant your response here (maybe I misinterpreted it)?

djc
it has some docs
do you know why H2Exec needs to know the output type of futures it executes?
LucioFranco
because spawn requires that iirc
or it originally required a output () before the joinhandle stuff aws introducded
(.. skip some stuff here ..)
seanmonstar
I think it still exists due to laziness.

[seanmonstar]

Oh, I was referring to Payload instead of HttpBody. My bad!

[djc]

So what's the purpose of having a separate H2Exec trait? (Motivating question: if I add H3 support, do I add an H3Exec trait?)

[djc]

So @seanmonstar (or others) any thoughts about this? I'm totally fine if you don't want to merge this and/or if I missed what these traits are trying to achieve. I'm happy to find some other way forward for H3 if you want to keep these traits -- it's just that the current setup seems to have more complexity than warranted (but, again, I don't have the context to understand what it's trying to do, and I didn't see any comments or documentation about this).

[seanmonstar]

These traits were just trying to reduce the noise on the bounds, since without them, we have E: Executor<crate::proto::h2::server::H2Stream<S, B>> or similar.

The exact name is less important.

[djc]

So is it necessary/useful to separate Executors that can execute H2Stream vs NewSvcExec? Otherwise we could have one trait Task for every type of task, and we get simple bounds. Alternatively, we could introduce an enum Task that just has several variants. Both of these proposals seem simpler (in that they don't require as much duplication across the code base) to make work.

[seanmonstar]

I don't understand the trait Task bound. The enum still ends up needing multiple generics, and some APIs don't need all variants, leaving filling in those generics to be weird.

[djc]

I was thinking instead of having different traits per type of task (H2Exec and NewSvcExec) we could instead have a single Task trait and then Executor impls can just accept any T: Task. Does that make sense?

[seanmonstar]

Are you suggesting that each of those futures would implement Task?

The point of those bounds is to forward the sendiness to the executor, such that you could have !Send services if the executor can spawn them.

[djc]

Are you suggesting that each of those futures would implement Task?

That's what this PR implements, because it seems surprising/complex in a way that doesn't seem warranted to have one trait per type that implements Exec.

The point of those bounds is to forward the sendiness to the executor, such that you could have !Send services if the executor can spawn them.

Ah, so now at least I better understand the goal of this code. Not sure I fully understand how that works yet, though. We have impl<F, B> H2Exec<F, B> for Exec for H2Stream<F, B>: Future<Output = ()> + Send + 'static and impl<E, F, B> H2Exec<F, B> for E for E: Executor<H2Stream<F, B>> + Clone, H2Stream<F, B>: Future<Output = ()>... so how does that forward sendiness to the executor? Because Exec is used to wrap executors that require Send futures and otherwise we fall back to E: Executor?

[djc]

A different approach might be just to turn H2Stream into an enum (HttpStream?) that could also hold an H3 stream (I think these will want the same generic parameters anyway).

[seanmonstar]

Exec is just the default one that uses tokio::spawn underneath. But the sendiness forwarding is what allows the single_threaded.rs example to work, by passing a tokio::task::LocalSet as the executor, and thus being allowed to use Rc inside the service or body.

[seanmonstar]

I'm not sure this is much different (E: H2Exec<S::Future, B> vs E: Executor<H2Stream<S::Future, B>>).

To your original question of whether we'd need an H3Exec, I think probably yes. But I don't think a user will ever need to worry about that.

[djc]

We might decide to rename H2Exec to HttpStreamExec (or some such), since the H3Stream would be quite similar anyway (in particular, requires the same generic arguments IIRC).