Skip to content
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

Dynamic Dispatch / Classic Wrapper for MakeService is surprisingly non-trivial #10

Open
Dessix opened this issue Oct 29, 2023 · 12 comments

Comments

@Dessix
Copy link

Dessix commented Oct 29, 2023

The Problem

With the current lack of object-safety on async traits1, Dynamic Dispatch on Service instances can only be achieved by converting to classic Tower traits for boxing.

With a single layer, into_classic(), tower::ServiceExt::boxed(), and a few extra where constraints are sufficient. But, with multiple layers, it seems to require not only async_fn_in_trait but also return_type_notation to achieve, which then massively convolutes usage at the call site.

Even with all of this, I've only managed to come up with a non-generic wrapper for my individual use case, which continues to be difficult to utilize due to a large number of impenetrable trait solver failures.


Feature Request

An API for correctly boxing via classic-service wrappers would be very helpful, especially if it can handle the multi-layer boxing problem2 involved in boxing a tower_async::MakeService.

If it ends up being nightly-only due to a requirement on return_type_notation, that would still be very nice even behind a feature flag.

Footnotes

  1. As mentioned in the RFC to stabilize it, with no active or draft RFC to add dyn-async-traits, so far as I am aware. The tracking issue for eventual work on this is Tracking Issue for Async Functions in Dyn Traits rust-lang/rust#107011.

  2. My attempt to do so with an intermediate MapResponse call to box the outgoing services was, as mentioned above, unsuccessful.

@GlenDC
Copy link
Member

GlenDC commented Nov 9, 2023

Given it is a feature request I assume it is so that you cannot contribute this yourself either due to lack of time or feeling stuck?

What’s the use case for this? As in my usage I don’t really need boxed services?

@Dessix
Copy link
Author

Dessix commented Nov 9, 2023

I got completely stuck in the process and ended up switching to standard Tower for orchestrating stacks of services, after converting them using the ClassicWrapper functionality.

I ended up needing dynamic dispatch in the architecture of a plugin system, but I also found that the Send-bounds bug in the compiler can be alleviated through boxing.

@GlenDC
Copy link
Member

GlenDC commented Nov 21, 2023

Ok thank you for taking the time @Dessix to report and elaborate on this. For now I have no time or motivation to really work on this, but I do welcome contributions about it.

As I said I am already working fine in production with tower-async as is and recently with the 0.2 release I also have support for hyper and improved the code further.

Next tower-async work that I'll undertake myself is hopefully in function of tower-rs/tower#753, with as goal to be able to start preparing the tower codebase to support async fn traitsdirectly. It is very possible that as part of the ongoing R&D for that goal that I or someone else can also tackle this issue as it is anyway something that is probably needed for a real solution that can be shipped in the actualtower` setting.

@Dessix
Copy link
Author

Dessix commented Nov 21, 2023

Works for me- though with the send bounds issues still active in the compiler, async trait fns being marked "stable" may have been a bit premature.

@GlenDC
Copy link
Member

GlenDC commented Nov 21, 2023

Well.. Not sure about that. For many use cases it is already possible.

E.g. in my case I do need it as long as stuff like hyper requires a classic Future trait. But once such crates also support async fn traits then for my use case I wouldn't need that send bounds at all.

But yeah I do agree that there are probably also plenty of use cases like yourself. That said I think the real stability will be there in Rust 2024 I guess. Until then it will be a bit awkward I think.

@GlenDC
Copy link
Member

GlenDC commented Dec 18, 2023

Hi @Dessix as I also need something like this for a small Web Service (http) router I am making,
would #12 be sufficient to close this ticket?

(No call(): Send Bounds are required)

@Dessix
Copy link
Author

Dessix commented Dec 18, 2023

That appears to viably cover my use cases; Is there a chance you'd be willing to provide a type alias for Boxed MakeService types, to reduce typing in that area? Otherwise, I suspect I'll be importing that declaration in a lot of places that consume this.

For future PRs, something like axum's IntoMakeService is difficult to declare outside of the defining crate for the trait, and might be easier to type-check from the library itself, as it won't need to handle proving return bounds on extension trait lifetimes. I'd say to put that in a second PR though- this one does its one purpose well as-is.

@GlenDC
Copy link
Member

GlenDC commented Dec 20, 2023

Would you be willing to contribute this? I'm not entirely sure I am following 😓 .

@GlenDC
Copy link
Member

GlenDC commented Dec 20, 2023

Hmmm... I think I was too optimistic...
I was trying to add in that branch a simple Http Router with my endpoint services being a BoxService...
I realise now that this thing won't be possible without being able to define stuff like call(): Send...

Which would make it very experimental for much longer...
As such I'm once again wondering what to do with this one. For now I'm going to drop this idea...

@Dessix
Copy link
Author

Dessix commented Dec 21, 2023

Mind flagging it under a nightly feature like pyo3 does and using that feature on it in the process?

@GlenDC
Copy link
Member

GlenDC commented Dec 21, 2023

Added a nightly feature in the current PR.

I think I screwed it up a bit so far though. Might need to have a fresh look at some point later.
Or perhaps you can help out in it a bit if you want it faster.

Problem is that the Send/Sync bounds are a requirement that leak through everywhere...
And as soon as you layer it seems to get into troubles... But might be messing something up.. somewhere... not sure.
You can see my hyper-http-router in that PR as a reference of what I'm implementing against. Perhaps it's a wrong approach to begin with or perhaps it's not even representative of what you're doing. You tell me.

@GlenDC
Copy link
Member

GlenDC commented Dec 23, 2023

After reading through the newest announcements at https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html and discussing it on other channels I think it is clear that the time is not yet ready for tower to be async. Implementing this fork has been a great learning experience for me, but for now I'm going to cut my losses on it, as it is starting to cost me a lot more in terms of resources then the benefits I was hoping to get out of it.

I do not plan to archive this repo however as I am continuing to opening this up to others to help experiment with this code, both by using it, as well as adding missing features or even making entirely new design proposals. All can be used to feed back into the rest of ecosystem and tower. Be it in the form of ideas or code.

In the meanwhile I'm going myself to switch back to tower itself so I can spend my tower-async time on more contribution time on tower itself.

Last reason why is because it became also clear that it might not even be certain that this kind of design approach of tower-async is the future of tower. As it might well in a very different direction. So for now, nice as it was, I'll no longer be actively developing on this repo.

I'll remain however still available to you and others who are interested in tower-async, and gladly mentor and help others with contributions to it :) Also still happy to have discussions about it over discord.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants