thruster-rs / Thruster

A fast, middleware based, web framework written in Rust
MIT License
1.07k stars 47 forks source link

Proposal: Using traits instead of static fn for middleware chains #151

Closed rakshith-ravi closed 3 years ago

rakshith-ravi commented 4 years ago

RFC:

I'm proposing moving middleware chains to using traits rather than static fns. It'll make it significantly easier to add objects to chains, rather than creating a new function for each chain item. Moreover, traits can respond dynamically rather than using a static function whose definition can't be changed.

This is similar to how Nickel.rs does it. You can take a look at that here: https://github.com/nickel-org/nickel.rs/blob/master/src/middleware.rs

trezm commented 4 years ago

I'm having trouble finding a concrete example in Nickel that uses structs and takes advantage of the trait. Can you modify your RFC with a few concrete examples of problems that this solves that are currently not solvable with just functions?

rakshith-ravi commented 4 years ago

This comes more from a need rather than a nice-to-have. My current use-case is building a http microservice for our company. We're trying to build a express-like http microservice that can handle routing and direct requests to the right container based on the request. I figured the best way to do that would be to build on top of this library.

Our microservice would involve endpoints like use, get, post, etc similar to what this library provides and would take in parameters of:

This would mean that the http module that we're trying to make would dynamically route requests to different containers based on the request being sent. Since the chain would be determined at runtime, we can't define the functions at compile time for the system to be able to use it.

Our resulting code would look something like this:

struct HttpRoute {
    pub type: RouteType, // GET, USE, POST, etc
    pub destination_container_id: String,
    pub destination_endpoint: String.
}

impl Middleware for HttpRoute {
    async fn invoke(&self, ctx: Context, next: NextHandler) -> Result<Response, Error> {
        containers
            .get(&self.destination_container_id)
            .unwrap()
            .get_endpoint(&destination_endpoint)
            .unwrap()
            .execute(&ctx)
            .await
            .unwrap()
    }
}

The execute(&ctx) part will basically involve calling an endpoint on another container (which is a microservice) and returning the result of that call. The chain will be dynamically be decided during runtime, by similar use, post, and get calls from other containers. Because of this, we can't determine the chain functions during compile time. Having a trait implement these would allow us to store multiple HttpRoutes in a Vec and then upon a call to start the server, we can add these traits / structs to the chain dynamically. Hope that helps.

EDIT: Oh, I forgot to mention, our microservice framework is open source. You can take a look at it here: bytesonus/juno

trezm commented 4 years ago

Your framework looks very cool! Super nice api!

I'm still a little unclear why this is a need though. I'm probably misunderstanding the nuance here, but I would think that you could do one of two things:

  1. Use a lazy_static CHashMap to store a list of listeners by endpoint
  2. Use crossbeam and channels to task that maintains a dynamic list of listeners, runs the dynamic chains, and then sends a message back to the request-specific thread.

The former is a bit easier, and that's very roughly how the file middleware works, since it's dynamically returning files from it's cache.

I'm also unclear how this would work on a dynamic level with how nickel does it. If I'm understanding correctly, each middleware would need to be mutable at some level in order to dynamically update paths. Nickel doesn't currently offer this, I imagine because they'd run into a similar issue that we would -- that is, if each request that flows through the system requires a lock on a single middleware, or a new instantiation of a middleware, the code becomes much slower and potentially more tangled.

One thought I'm having now, and this was probably your point that I missed earlier, is that the idea is to build the tree dynamically before start is called on the thruster server, but then lock it into place once it has been started. This is currently doable by generating your own MiddlewareChain to pass into one of the get, post, etc. functions. Without using macros, it would look something like this:

let chain = Chain::new(vec![&static_ref_1, &static_ref_2, &static_ref_3]);
let m = MiddlewareChain {
    chain,
    assigned: true,
};
app.get("/whatever", m);

Where static_ref_n can be either defined as a static reference to a fn, i.e. static some_fun_stat: Middleware<C> = some_fun;, or simply passed by argument with the type &'static Middleware<C>.

The annoying part here isn't really that they're individual functions, it's the requirement around making sure the functions have a static lifetime so they can be used in pin boxes that thruster can pass around.

Does this all make sense? If I'm still misunderstanding the use case, let me know!

rakshith-ravi commented 4 years ago

I think it's easier for me to explain with an example, so here's a boilerplate of the container that we're trying to build. This is on top of the previous example that I gave you. I hope this helps you understand why I can't use static fns:

async fn main() {
    let app = thruster::new_app(); // I forgot the syntax.
    let mut container = Container::with_name("http", "1.0.0");
    let mut chain = Vec::<HttpConfig>::new(); //HttpConfig implements the trait
    container.expose_endpoint("use", |data| {
        chain.push(data.http_config);
    });
    container.expose_endpoint("get", |data| {
        chain.push(data.http_config);
    });
    container.expose_endpoint("post", |data| {
        chain.push(data.http_config);
    });
    container.expose_endpoint("listen", |data| {
        for config in chain.iter() {
            if config.type == RouteType::Use {
                app.use(config);
            } else if config.type == RouteType::Get {
                app.get(config);
            } // ... and so on for other http methods
        }
        app.start(data.bind_addr).await;
    });
    container.listen().await;
}

Now, in a different container, the user can do:

async fn main() {
    let mut container = Container::with_name("my-company-api", "1.0.0");
    container.call_endpoint("http.use", |req| {
        Ok() // Do jwt authentication or something here
    });
    container.call_endpoint("http.get", "/hello/world", |req| {
        Ok("<html>Supp world?</html>")
    });
    container.call_endpoint("http.listen");
    container.listen().await;
}

Now, naturally most of the above is pseudo-code since it's all just to help you understand how it works, but you get the point.

Notice how the chain is constructed dynamically based on what the other container calls? The http microservice will basically expose a "microserviced" version of this library. This makes it easy to construct chains if the chains are a trait. In the case of static fns, the chain cannot be constructed since there are a dynamic number of middleware functions each that does something dynamically.

If this is still not clear, I'd be happy to get on a voice channel on discord to help you understand the exact scenario I'm trying to tackle.

rakshith-ravi commented 4 years ago

Now I'd also like to add that I understand that making a huge change like this is not a simple thing, and I understand your inertia to make such a move, I'd be too. What I'd recommend is having a trait and a default implementation of the trait that uses a static fn. That way, the API wouldn't change much, but it would also allow other people to extend it if required