plabayo
revisit Service API (core) design in 0.3 milestone
As rama is "maturing", we start to get more end user experience and also collect more feedback from other users. This didn't make the 0.2.x cut as it's an impactful change that will require some iteration to get right. But for now this is more or less how I see it.
pub trait Service<Request>: Sized + Send + Sync + 'static {
type Response: Send + 'static;
type Error: Send + Sync + 'static;
fn serve(
&self,
req: Request,
) -> impl Future<Output = Result<Self::Response, Self::Error>> + Send + '_;
fn boxed(self) -> BoxService<Request, Self::Response, Self::Error> {
BoxService {
inner: Box::new(self),
}
}
}
in contrast to https://docs.rs/rama/latest/rama/trait.Service.html. The change here is that Context would be removed, if we indeed go through with this. I want to be clear this isn't a certainty yet. And this issue is mostly to start the discussion and iteration of ideas on how we might want to tackle this and anything related in 0.3.
So why would we want to get rid of Context, if at all?
This would make rama compatible with a future version of tower. And while rama might always have its own services, it might make it at least possible for us to make it possible to use tower services in rama and rama services in tower context is today used for three things.
1. State
This is the big one, but as rama starts to become more mature and I have more experience with it as an end user I start to think that we pay a high price for that static state. Sure it's nice, but it's also very limited in use. The real killer is to have it in web endpoint services, which is a very specific use case. And other case where I use it is to have static stuff for creating services downstream. For the first we might be able to make it still work but more the axum-approach where we perhaps tie that directly into the http server somehow and still allow it to be accessed as an extractor. Not sure how that would work though, would need some figuring out. The latter I am not convinced we really need static types for that. Might as well just put that stuff in extensions if needed or use a more functional approach by using global functions unique to the app to get access to this information. At the other hand the price for this state is really high even though in 80% of places we do not use it, you do always have to take it into account.
Why was state introduced?
State didn't come out of the blue and the current context is still pretty solid I think, coming out of many iterations of what rama now is. The biggest reason why it made the cut is because it made it very easy and clear to pass state through in a type-static manner, in contrast to anything like extensions which is dynamic and runtime-uncertain. In contrast to that something like State will stop your program from compiling all together if it doesn't contain something that you expect it to contain. To make it even more flexible there is the AsRef / AsMut trait pair that you can use to work with any state that can give you what you expect (e.g. some property or db handler of some kind).
A typical use case of state is web service endpoints. And in fact that is the only context in which I've actually used it so far, which is one of the reasons why I started to think more and more that perhaps we pay a very high price (as context and its state are to be handled by every single service out there, even though most only ever use extensions as the AsRef / AsMut concept is pretty limited in possibilities and also because a lot of stuff is actually really optional in nature anyway.
https://github.com/plabayo/rama/blob/9a9e309156aad35be67dbd7af2e2d11820edcfab/rama-cli/src/cmd/fp/state.rs#L7-L14 <= there you can see a typical example from the <fp.ramaproxy.org> web service. Given how tight this use is coupled to web services we might better find a way to just couple such static (opt-in) state directly to stuff like the http server (and in fact today it already contains that logic, it's just that it injects it into the context's state, but it could instead do something different. What I do not know however).
Sometimes proxy services also use it to pass information on how to create certain egress services in the middle of a proxy flow:
pub(super) http_firewall_enabled: bool,
pub(super) transport_firewall_enabled: bool,
This however is a less strong reason as you could also instead inject these into the extensions. Or even differently expose them as public global functions. Because in the end these are all end user cases anyway, so I would think it's less dirty to expose these as global statics, especially if they are behind a clean functional API. Even for configs loaded from config files or cli args you can still expose them as static (behind functional API) by removing such variables out of the tracking and leak them as statics. A lot is possible like that and imho still a lot better than a high price to pay for such limited use case.
2. Executor
This one is mostly there so one can easily spawn graceful-aware tasks. Now you gotta know that in the past even Dns was part of the context for similar reasons. And I pulled that out of there as it was a pretty edge case thing to be in there. I think in the same way we can pull that executor out of context (or rather it's not a good enough reason to have context for this). Instead we could put a Guard in the Extensions and provide a function in rama::rt that would be somthing like fn spawn<F, E: AsRef<Extensions>>(r: E, task: F>) which would use the guard if available and otherwise just tokio::spawn. So same same, but without the need to drag a context along.
3. Extensions
This one has always itched me a bit but so far I've been ok with it. It essentially means that as soon as you enter http world you have 2 extensions collections. Instead I imagine a world where we could have just only the one in the http request. That would mean we can also make a Request<Stream> type for transport layers, that contains a Stream and the Extensions. Similar to the http Request. Even better is if we can somehow use the same Extensions type as that would mean we can forward the extensions to the http layer if we cross that ISO layer. To make that happen ideally the extensions type of http is pulled out of the http crate though as otherwise we would always need to pull that crate. And other idea is of course to instead put the transport extensions as a single value in the http extensions. That would not require any changes in the http crate and be perhaps even cleaner.
If you take this all together I think there is enough in here that we can perhaps get rid of that Context and make our life a whole lot easier. https://docs.rs/tokio/latest/tokio/macro.task_local.html is another possible way to put static state, but haven't played with it yet.
Again this is for now just how I think about it, and there's still plenty of time before we would even start any work in this direction. First I still want to focus on getting 0.2 out of the door, and 1 or 2 releases after that as 0.2.1 and perhaps also still a 0.2.2. This because:
- there are still many nice features missing that I want to have added first before changing some fundamental designs that anyway aren't a real blocker for now
- but also this gives people plenty of time to chip in, and also other ecosystems to continue evolve around us (e.g. how will tower shape up by then, etc...)
This issue is mostly for now a tracker issue and a central place to chip in ideas, provide use cases for one argument or another, and to have a healthy long ongoing discussion.
Hyperium main maintainer already made clear that Extensions of http will not be decoupled. Which I think is understandable and fine. What I think it will mean is that if we go this road we would still keep our extensions and perhaps provide traits such as these:
trait ExtensionsRef {
// get data out of extensions or something contains extensions
}
trait ExtensionsMut: ExtensionsRef {
// insert data into extensions or something that has extensions
}
That could be implemented for our Extensions, but also http::Extensions, http::request::{Request, Parts}, http::response::{Response, Parts}, transport::Request, etc... This way the core layers can still easily inject stuff into extensions. Or get stuff out.
Only tricky part is the Orphan rule... We wouldn't be able to implement that trait in rama-http on the http types given we do not own those types (we re-export them) and the traits come from another crate as well (would be solved if it's a mono-crate (again)).
So yeah... Orphan rule is tricky here. If you want to make such traits work. Than again, might even work without such traits at all.
@soundofspace my current plan is to release 0.2 without this. But aim for a 0.3 within a month or so of 0.2, so we can do some minor breaking changes of stuff we didn't notice and also do this big change. This way we also have a record (0.2) of before the context removal.
This is how I currently see it:
- the state is pretty much only used in a web server context because there you control the endpoint services
- as such we can just make a pattern to have state in a web server and than can be smuggled to the endpoint services via extensions
- the state can be actually tied to a
StateFactorywhich:- by default does nothing
() - can also be done for a
S: Clonewhich is a factory on itself - or an actual impl (incl a Fn) which produces such state
- by default does nothing
- the state can be actually tied to a
Everything else we already do today with extensions.
This does mean that we'll:
- provide a
TransportRequestfor stream requests so that we can have extensions there as well - move https://docs.rs/rama/latest/rama/struct.Context.html#method.executor to extensions
- revive the concept of (link-chained) Parent extensions with a ExtensionChain trait to provide the option of also checking if there's a
Parent(Extension), as for example the transport's extensions would be a parent of the http extensions.
And I think that's about it. Pretty simple and solid, it will make most of the code a lot simpler. That said I do think it makes sense to first release 0.2, as long as we are open to make another major release (0.3) very quickly after that (within a month or so)
@GlenDC I'm going to pick this one up. Plan is to split up the work in multiple stages. Will probably follow these steps, but depending on how it goes it might change (or steps combined/split):
- Research/implement logic needed to replace current usages of state (http webservers, global state...) and migrate to these new solutions
- Nothing should be using State now so remove it from Context and all where clauses
- Unifying Extensions logic for Request/Streams/AnyInput/Response...
- Move all extensions from context to Request/... extensions
- We should now be able to remove Context completely
- Extra things such as parent extensions, ...
If there are major decisions or multiple options I will report back here or open a draft PR so we can discuss further
@GlenDC First thing I will tackle is global state. Think loggers, database, storage...
Some solutions I see there but not limited to:
Examples for examples/http_key_value_store.rs
struct AppState {
db: RwLock<HashMap<String, bytes::Bytes>>,
}
/// a service_fn can be a regular fn, instead of a closure
async fn list_keys(ctx: Context<AppState>) -> impl IntoResponse {
ctx.state().db.read().await.keys().fold(String::new(), |a, b| {
if a.is_empty() {
b.clone()
} else {
format!("{a}, {b}")
}
})
}
- Global state pattern (can be renamed to support multiple states if needed/wanted)
static STATE: OnceLock<AppState> = OnceLock::new();
fn init_state(state: AppState) {
STATE.set(state).unwrap();
}
fn state() -> &'static AppState {
STATE.get().unwrap()
}
async fn main() {
..
init_state(AppState::default());
..
}
/// a service_fn can be a regular fn, instead of a closure
async fn list_keys(ctx: Context<()>) -> impl IntoResponse {
state().db.read().await.keys().fold(String::new(), |a, b| {
if a.is_empty() {
b.clone()
} else {
format!("{a}, {b}")
}
})
}
- Global state(s) behind a trait, this enforces this pattern more and has native support for multiple types
trait GlobalState {
fn into_global(self) -> &'static Self;
fn global() -> &'static Self;
}
static GLOBAL_APP_STATE: OnceLock<AppState> = OnceLock::new();
impl GlobalState for AppState {
fn into_global(self) -> &'static Self {
GLOBAL_APP_STATE.get_or_init(|| self)
}
fn global() -> &'static Self {
GLOBAL_APP_STATE.get().unwrap()
}
}
async fn main() {
..
AppState::default().into_global();
..
}
/// a service_fn can be a regular fn, instead of a closure
async fn list_keys(ctx: Context<()>) -> impl IntoResponse {
AppState::global()
.db
.read()
.await
.keys()
.fold(String::new(), |a, b| {
if a.is_empty() {
b.clone()
} else {
format!("{a}, {b}")
}
})
}
There are lots of other options here:
- Insert state into extensions. This feels like an anti-pattern for global state, but has the advantage that you have more control of what is shared between certain runs. I do think this is a useful solution, just not for global state
- Use lazy static. This is harder to create with dynamic inputs and share between the entire codebase
- A lot more...
I currently have very mixed feelings. One the one had solution 1 is very very simple which is a huge advantage always, but on the other hand, solution 2 is a bit more elegant and can be useful to enforce specific patterns (which makes me lean slightly towards this).
What do you think about this, or are you leaning towards something completely else? (Only focus here is global state)
I am a bit confused why this is even something to be solved? Global state is already possible today and what is me concerned lives outside the realm of Rama. Don't see why we would need to support anything for that as you can easily do that with just the std lib without needing to invent anything? In fact I already use that sometimes.
The state we talk about here is not global, it's specific to a service.
So this feels like a problem that doesn't need to be solved? Unless I miss something. We could add an example about it for sure, but beyond that not sure what this requires.
It's certainly not something that I wish to promote as the default as I honestly hate globals unless they are the only thing that makes sense. Because once you want to run two instances of the same thing globals like this no longer work out of the box anyway. And again, also not sure what this abstraction adds of value either.
But then again... maybe I'm missing something here?
I think the topics that need to be solved:
- allow "local" typed state where desired:
- e.g in web services, which ideally are also available as extractors in endpoints
- also check where else in server-like environments we want such typed state
- ensure non-http services still have extensions (e.g. for transport layer that means we need a wrapper type probably)
- ensure extensions can be chained/nested (parent etc)
- ensure extensions work with different types (we have our own extensions type in rama-core, but the http layer works with extensions type from http crate (re-exported via rama-http-types))
- ability for extensions to also get returned (fully, partly or fine-grained) with response
- move stuff like graceful guard to extensions that are then used to spawn instead of it being part of Context
Other typed state we do not have to solve as that is inheret to structured programming, and comes for free with rust. E.g. struct properties of Service impl's and others.
Similar to this is why I don't think we really need to provide a solution for global state, as that is already possible without anything required from our side. I do still think an example that makes use of global state is a good idea. But that's about it I think. Do correct me if needed.
@soundofspace we had in past some issues around stale state due to the concept of RequestContext, e.g. example usage:
match ctx
.get_or_try_insert_with_ctx::<RequestContext, _>(|ctx| (ctx, &req).try_into())
.map(|ctx| ctx.authority.clone())
{
Ok(authority) => {
tracing::info!(
server.address = %authority.host(),
server.port = %authority.port(),
"accept CONNECT (lazy): insert proxy target into context",
);
ctx.insert(ProxyTarget(authority));
}
Err(err) => {
tracing::error!("error extracting authority: {err:?}");
return Err(StatusCode::BAD_REQUEST.into_response());
}
}
Was thinking, once extensions are always part of the request type, we can simplify a lot and I would avoid trying to be smart about caching (caching is hard) and always compute instead. This avoids anything stale. Where we need cached intermediae state it can be because it is another extension that can be cached (e.g. overwrite of authority or w/e), but that's up to the request tpe to be able to deal with that.
So instead I was thinking to perhaps come up with new traits:
//! somewhere in rama-net
trait AsRequestApplicationProtocol {
fn as_request_application_protocol(&self) -> Protocol;
}
trait AsRequestAuthority {
fn as_request_authority(&self) -> Authority;
}
#[cfg(feature = "http")]
trait AsRequestUrl {
fn as_request_url(&self) -> Url;
}
This all as stateless as possible. It would also help for future proofing things like #698 as an FTP Request would also fit this.
But even for all existing code it would make it a whole lot simpler and always work with the latest state without having to be worried that a previously computed authority has impact on the current authority...
@soundofspace we had in past some issues around stale state due to the concept of
RequestContext, e.g. example usage:match ctx .get_or_try_insert_with_ctx::<RequestContext, _>(|ctx| (ctx, &req).try_into()) .map(|ctx| ctx.authority.clone()) { Ok(authority) => { tracing::info!( server.address = %authority.host(), server.port = %authority.port(), "accept CONNECT (lazy): insert proxy target into context", ); ctx.insert(ProxyTarget(authority)); } Err(err) => { tracing::error!("error extracting authority: {err:?}"); return Err(StatusCode::BAD_REQUEST.into_response()); } }Was thinking, once extensions are always part of the request type, we can simplify a lot and I would avoid trying to be smart about caching (caching is hard) and always compute instead. This avoids anything stale. Where we need cached intermediae state it can be because it is another extension that can be cached (e.g. overwrite of authority or w/e), but that's up to the request tpe to be able to deal with that.
So instead I was thinking to perhaps come up with new traits:
//! somewhere in rama-net
trait AsRequestApplicationProtocol { fn as_request_application_protocol(&self) -> Protocol; }
trait AsRequestAuthority { fn as_request_authority(&self) -> Authority; }
#[cfg(feature = "http")] trait AsRequestUrl { fn as_request_url(&self) -> Url; }
This all as stateless as possible. It would also help for future proofing things like #698 as an FTP Request would also fit this.
But even for all existing code it would make it a whole lot simpler and always work with the latest state without having to be worried that a previously computed authority has impact on the current authority...
This specific redesign is becoming urgent for me. Do you have time to pick this up @soundofspace this week, else I might pick it up in the next days.
I think the goal is that we have a trait to get
authority(&self) -> Cow<'_, Authority>;
host(&self) -> Cow<'_, Host>;
port(&self) -> Option<u16>]
uri(&self) -> Cow<'_, Uri>;
This way everything that needs one or more of these items always gets the correct info.
It would replace the RequestContext for Uri wise. and also means we no longer need
the "hacky" request_uri function recently introduced (yesterday or day before).
For the Application Protocol we need to think a bit harder, as that info is also used by the uri method above to deduce the scheme in case info is not found... Problem is that currently this cannot be correctly detected always:
- by default might assume http
- but what if it really is https
- you might check for
SecureTransportbut that is also there in case we are in https proxy tunnel
- you might check for
This made me think it might be required for us to be able to chain extensions so that when checking for what protocol it might be, we can recurse through the extensions.
Meaning that if we have TLS and then HTTP, then the recursion would first think it's http, and then that turns into https when it encounters the tls. But important is that if a TLS Acceptor is in function a TLS Proxy Tunnel that this would not influence the application protocol. So It's an example (similar in spirit to Tls Config Builder I imagine) where we need to be able from any part in the layer to built upon the chain in a non-destructive manner, leaving breadcrumbs so we can in a very pure way get at any point of the progress the correct current protocol as that is information that our stack does have (given it handles the protocol), but which is not always available in the request itself. E.g. http/1.1 requests are completely clueless wether or not they went over tls. Just to given an example.
Either way, this is becoming urgent, as I'm getting issues now and then with the current state of affairs and also for security reasons this needs to become consistent.
Also let me know, @soundofspace, if I better create a separate ticket for getting the resource identifier (uri/authority/...) information and deduce application protocol. Might be getting a bit much this ticket But all good for me, if good for you.
@GlenDC I wont have time probably to finish this (request_context) in the very near future so please feel free to take that over. I also believe that this deserves an issue of it's own as it seems quite easy on first sight, but it's the exact opposite once you include all the special cases.
Do tag me on that issue or a draft PR once you have an idea of how you will fix this. Parent extensions is also pretty much finished and PR for that should be live any time now in case you need it for this. But if something else is needed or we need some way of chaining things in special way let me know and we can see what we can figure out, but it does look like we need to invest some time in this pattern as it will keep coming back for different use cases
Good job @soundofspace this was a big one! Love the work done here. Great teamwork