-
Notifications
You must be signed in to change notification settings - Fork 282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
breaking change in tower design (0.6 or beyond): first class support for async fn traits #753
Comments
I think cutting scope for the sake of experimentation makes sense, but I'd like to push back on this a little bit. Removing With the current design, functionality like pooling, routing, and load balancing can all be represented with the This is not to say that I'm not open to making drastic changes to the |
I can stand behind all of that @hawkw. I'll wait until more feedback from you and others so it's more clear what further experimentation can aid this discussion. The original reason that I cut out That said, I do lean more into the camp of providing that functionality as additional utility code that people can integrate in their service calls. But yeah also there I do agree that if we go that road that we indeed need to make sure that it is possible indeed. |
I agree with @hawkw that if those things are removed, some sort of design for @hawkw I do have one idea, that is probably sufficiently deep enough that it could become its own issue, but I'll dangle it here. I noticed that hyper v0.14's client pool was essentially a |
This is what I've been doing in my implementation atop While |
It is worth pointing out that it's trivial to go from a classic future tower service to the new pub trait FutureService<Request> {
type Response;
type Error;
type Future: Future<Output = Result<Self::Response, Self::Error>>;
fn poll_ready(&self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>>;
fn call(&self, req: Request) -> Self::Future;
} A pub trait Service<Request> {
type Response;
type Error;
fn call(
&self,
req: Request,
) -> impl std::future::Future<Output = Result<Self::Response, Self::Error>>;
}
This is of course not interesting if we care only about that conversion. But it does mean that middleware at the "beginning" of your stack could require a That said, even if you want to provide the kind of features that The bigger issue is that switching from That said, I've always believed that the expressiveness of Tower comes from the fact that the I do mean that if we want to keep that support I'm all for it, but if so it should be by facilitating those users with utility code and proper documentation how it can be easily achieved. This way it would still be there, but totally opt-in. I don't have a specific design proposal on that one, but it would essentially be some kind of "higher order" Service. Similar to |
That is something I didn't think about during our previous chats on Discord. Not being able to box services is a deal breaker for axum 😞 No boxing would make the |
I get you there. I am for now doing That said, I'm pretty certain that we can work around this. There are indirect ways to box. And thus use cases like Axum should be possible. Only sad part is that it will require that we can use things like |
The fact that there is (currently) no way to introduce bounds like It's worth noting that the current design, with an associated future type, can be used with unboxed async blocks using #![feature(type_alias_in_trait)]
impl Service<Req> for MyService {
type Response = Response;
type Error = Error;
// using the `type-alias-in-trait` feature, we can return an `async` block future
// without boxing it.
type Future = impl Future<Output = Result<Self::Response, Self::Error>>;
fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
// ...
}
fn call(&mut self, request: Req) -> Self::Future {
// we can still use async-await to implement `call`!
async move {
// do stuff...
}
}
} Downstream code can depend on this type Future = impl Future<Output = Result<Self::Response, Self::Error>> + Send + 'static; This allows code to introduce additional trait bounds on a |
Unfortunately, I think being able to introduce trait bounds on We probably can't seriously consider an When RTN is available on stable Rust, we'll probably be able to reconsider this. |
Potentially, we could consider other breaking changes to the |
To be honest, I didn't consider the As much as I have a forbidden love for So given that this wouldn't need any change from tower, we could perhaps instead focus on a People like myself could then already use the nightly feature to already allow async fn like implementations where this is needed / desired. And all is well? Or am I seeing things to optimistic / missing potential use cases here? |
In my opinion, this is probably the best approach, since we get to maintain our current flexibility while still allowing use with unboxed async/await when the user is willing to opt-in to nightly. Perhaps we should add documentation explicitly demonstrating use with Footnotes
|
I agree that this might be the best option indeed. Saves a lot of potential pain as well. Also agreed that documenting this is a good idea. We can add that footnote indeed, even though seems obvious that nightly features limit your library's use? I'm curious what others have to say about this change of heart. |
It looks like there is a typo |
@hawkw I made a little playground of an earlier playground I had shared in the
It works excellent. Very easy to use and can be used as-is in the existing eco system. Regular tower services would work and implementing tower services where you do need opaque async services (e.g. to call Honestly as much as I would like super magical Only breaking changes I did do in the
I am still not too happy with the fact that That said... Maybe I am missing enormous potential and usage and benefits of And with that I am looking forward to the feedback of all of you and see how we can move forward this discussion. I am thrilled at the future now more then ever, as the approach that @hawkw suggest does seem to work very fine, and would allow me to drop the |
I agree this seems like an interesting path that's worth considering!
I'm not sure that's possible. Ideally a default implement would be I'm not really sure how I feel about |
Well put @davidpdrsn , good catch. That would make a default implementation of |
Cross-referencing another |
FYI I did start to experiment with a boxed You can find the open blocked (by not working) PR at plabayo/tower-async#12 another point for impl associated type (which sadly doesn't have a specific/real plan towards stability, at least not one with an estimated date of arrival) |
Another plot twist... In the 5th major iteration on the Rama project I moved away from tower-async... First I was planning to go down the route of providing my own Service but bridging it to Tower, as to be tower (classic) compatible... I then realised however how big of a mess that is and I especially don't like it as it hinges so much on the assumption that I do not want or need to support the The result can be seen at https://github.com/plabayo/rama/blob/985bb11cad8e86e78776d9cbbd521890aaef27da/src/service/svc.rs and so far I'm pretty happy with it:
The trait looks as follows:
The In this new design (sadly for now completely separated away from the tower ecosystem, which I dearly admire and like) I do not have to clone except for a couple of cases:
And that's it. In most places I don't need it at all... And this all thanks to the fact that all services now require to be First I was only requiring it to be I'm now at a point where I can finally build the more interesting pieces of Rama. And while I'm sad that for now I am moving further in this project without You can browse the codebase for seeing how it is used and plays out, in case it might inspire the future direction of tower, or perhaps might inspire how to not do it. Either way I stay available on GitHub and Discord for collaboration and elaboration. Happy new year all and take care. Extra (not important):
|
+1 to replacing Returning I would not add a
As someone who's using hyper+tower for clients, this naming doesn't sound right. The new name only makes sense when using tower for servers. Calling |
Hi, I would like to open this work to start the discussion and start to make progress towards getting tower ready for when
async fn
in traits andimpl
return values for traits are allowed.As I really needed this kind of first class support I have a completely ported version of
tower
ready at https://github.com/plabayo/tower-async.tower-async
can be serve as a starting point of discussion. And While it can reflect the design we take, I am also fine for it to be nothing more then a starter for this discussion and be completely ignored after thattowers
(and as many versions as it takes) to try out the ideas. I am more then happy to use that repo as a playground, and I'm actually using it in production, so you would get actual use feedback in myrama
project :)I took a couple of drastic changes:
Service
. But that is obvious as it's the entire idea of this proposal. You can see that trait at: https://github.com/plabayo/tower-async/blob/6680bc9422083d893c42d5c5a0a28293bf10f281/tower-async-service/src/lib.rs#L196-L209async fn
support you'll notice that:poll_ready
: see the FAQ: https://github.com/plabayo/tower-async#faq, happy to discuss more. Also again, just my current POV, happy to change it if this is not shared with maintainers&mut self
in&self
. This is not a requirement and my first ported version did still have&mut
, however:&mut
, and making it&
reflects that direction and also gives a simpler lifehyper (v1)
;poll_ready
. e.g. anything related to load balancing and the like. This is on purpose as I didn't have an eed for it, and I think it's out of scope. I have ideas on how we can support it by providing such code but making it that users would integrate it in either theMakeService
stuff or as utility code that they would inject themselves in theircall
functions. Or have services that can pool other services etc etc. But again I didn't have a need for it or desire, so honestly I didn't push any of those ideas further and just dropped it.How to play with my experimental tower-async version?
The port of
tower
andtower-http
is completely done and can be used now using thetower-async
andtower-async-http
crates.You can use
tower-async-bridge
crate to interface with classicTower
interfaces (to and from).You can use
tower-async-hyper
to interface with the "low level"hyper (v1)
library.All crates are published on crates.io: https://crates.io/search?q=tower-async
How is my experience?
Great. It works and I'm using it in production. I'm also working on
rama
(https://github.com/plabayo/rama) where I'll have a similar production-ready setting ready as open source. But that is still early days.Seems that all the design works just fine and with the current stability plans it means that
tower-async
andtower-async-http
will be ready for stable rust this year.Open Problems
As my production use proves I have no issues/problems any longer that block me. There are however none the less still open problems.
Boxed Services is non trivial
Open issue: plabayo/tower-async#10
This can be solved and I might add it as an experiment to tower-async. But the only way I see how for now is using stuff like
call(): Send
, which... requires nightly. Dyn trait objects with async is not in active progress and not even sure this will be something that in the 2024 edition will be.I need exactly this nightly feature for my current
tower-async-bridge
andtower-async-hyper
crates. This makes me also fearful and unsure if we can really provide stable support forhyper
anytime soon by only adding code tohyper-util
. Because in order to implementhyper::service::Service
we will always require a boxedFuture
. This is only possible with thecall(): Send
nightly feature, which is not stable anytime soon.The only hope is that someone can help me figure a way out to add support for
async fn Tower trait
inhyper-util
by making use of theservice_fn
approach thathyper
allows. If we can internally withinhyper-util
use that, then we do not need a boxed future and thus also not thecall(): Send
syntax. I have a feeling however that this is only possible once the next described problem (rust-lang/rust#114142) is resolved.Trait Resolving is not flawless (e.g. might not deduce the future is
Send
)Open issue: rust-lang/rust#114142
Example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=df177519275726a7df18045cf90a59a9
You can come in situations where a higher order function cannot correctly deduce that a future of an async fn trait implementation is Send. A work around for this is turbofish notation to explicitly declare the type that is passed in. This is however not always possible and makes usage also more awkward when you run into this.
So far however I've not come into a situation where this work around does not work. If you need it I would suggest to hide it behind a central point as much as possible so that the rest of your code can be as ergonomic as possible.
This problem and its workaround were also documented since
0.1
of tower-async at: https://github.com/plabayo/tower-async#faq.Can we run everything in Stable?
Yes. However, currently there is no
async fn
trait support inhyper
orhyper-util
. So in order to make something liketower-async-hyper
work I currently need to be able to define aFuture
type and that requires me toBox
it. This requires me to use thecall((): Send
notation (however you call it) and this is going to be a nightly only feature for much longer.The good news is that this shouldn't be needed for an official async-fn ready
tower
version as we can then just implement official tower support withasync fn trait support
directly intohyper-util
which would allow us to drop the Boxed Future and thus also the need for Nightly Rust.Open for feedback
To iterate some of the above.
These are all just proposals though. I am open to any feedback, a totally different direction. I can do more experimentation if you want a different direction (completely or partly). Honestly fully open about it all. I just already needed it as for my purposes (where I do deal with plenty of async fn usage in my middlewares it was becoming a pain in the ass to hand roll these futures myself. If everything in Rust was manual futures it would be a lot easier, and I don't mind the work. But given that some stuff (e.g. plenty of tokio utility code) works with async fn that became very awkward and painful very soon.
tower-async is not meant to be permanent or me wanting it this way or no way. It was just unblock myself. A realisation made in the 900th iteration of
rama
. After struggling a lot with classic tower in many iterations before that.Prior work
@LucioFranco had apparently already added a case study for
Tower
in the context of the async fn trait RFC.Link: https://rust-lang.github.io/async-fundamentals-initiative/evaluation/case-studies/tower.html
I didn't know of its existence until I already have version
0.1
of tower-async. I did know about it prior to implementing version0.2
. As far as I can see it came to very similar conclusions as I have.It also proposes some potential intermediate solutions, but so far I have not had a need for it.
Next Steps
Most important I think is that the maintainers align on a vision of how they see
tower
.tower-async
can be an inspiration for this, and its design can even be taken as is. But even if none of its designs are taken at all, at least it might hopefully teach some lessons on how to to it different in that case.Either way, this is is how I see it progress after initial discussions and alignments:
0.3
oftower-async
to actually implement these ideas and also test them in production use.This can be seen as a typical design iteration loop, where we cycle through as much as needed and in any desired order.
tower-async
can be seen as a playground for "final" (hopeful) designs that we think are worth testing in production.The final step would then be porting
tower-async
back intotower
andtower-http
.Once that is complete the ecosystem can also start adopting it, which we could kickstart by providing first class support for it in
hyper-util
. I would obviously myself also start usingtower
once again (instead oftower-async
) intorama
. Other maintainers, such as the ones ofaxum
can also help by migrating to this.This requires no change of
hyper
and neither breaks any 1.0 promises in such systems, as we can provide bridge functionality for this newtower
design by adding it tohyper-util
, similar to hyperium/hyper-util#46Timeline
A proposed timeline would be get a version of
tower
out of the door with this in the next months. By then Rust has already stable support for all required features.The text was updated successfully, but these errors were encountered: