Proposal: Introduce the notion of a middleware stack

[skade]

Currently, the type of a collection of middlewares is Vec<Arc<dyn Middleware<Data> + Send + Sync>>. I think it would make sense to introduce a proper MiddlewareStack type that itself implements Middleware. This allows better sharing of middleware instances and the way to address them, for example for debugging.

[RoGryza]

I'd like to take this to start contributing to tide. Seems pretty simple:

impl<Data> Iterator<Item=&Middleware<Data>> for MiddlewareStack<Data> {
    // ...
}

impl<Data> FromIterator<Item=dyn Middleware<Data> + Send + Sync> for MiddlewareStack<Data> {
    // ...
}

impl<Data> Middleware<Data> for MiddlewareStack<Data> {
    fn handle(/* ... */) -> FutureObj<'a, Response> {
        // Apply internal middlewares in order..
    }
}

Anything I'm missing?

[aturon]

@RoGryza seems like a good starting point! Feel free to make a PR, and then we can build on that over time (with introspection etc)

[devashishdxt]

Is this still valid? If yes, is anyone working on this? If not, I'd like to start working on this.
@RoGryza
@skade
@aturon

[RoGryza]

Sorry for the delay, I hacked something together that works but I've introduced some allocations that could be eliminated and never got time to work on it again. I'll submit a PR once I get home from work in order to ask for help

[aturon]

This is worth revisiting now that #156 has landed -- anybody want to take it back up?

[devashishdxt]

I can take a look if no one else is already working on it.

[manuelmauro]

I am taking a look at this and came up with the following

pub struct MiddlewareStack<State> {
    middlewares: Vec<Arc<dyn Middleware<State> + Send + Sync>>,
}

impl<State> MiddlewareStack<State> {
    pub fn new() -> MiddlewareStack<State> {
        MiddlewareStack {
            middlewares: Vec::new(),
        }
    }

    pub fn push(&mut self, m: impl Middleware<State>) {
        self.middlewares.push(Arc::new(m));
    }
}

impl<State: 'static> Middleware<State> for MiddlewareStack<State> {
    fn handle<'a>(&'a self, cx: Context<State>, next: Next<'a, State>) -> BoxFuture<'a, Response> {
        if let Some((head, tail)) = self.middlewares.split_first() {
            let next = Next {
                endpoint: next.endpoint,
                next_middleware: tail, // todo: append middlewares from next
            };
            head.handle(cx, next)
        } else {
            next.run(cx)
        }
    }
}

But I got in trouble with the incompatible types of MiddlewareStack.middlewares and Next.next_middlewares
Are there some strong bounds on the type of Next.next_middlewares that I should keep into account or it is possible to work on that?

[skade]

I don't know of any bounds that next would need. I'm not even sure if the Stack itself needs the 'static bound. (an external user could apply it)

[manuelmauro]

Thanks @skade, I cleaned up my previous attempt.

/// Middleware stack.
pub struct MiddlewareStack<State> {
    stack: Vec<Arc<dyn Middleware<State>>>,
}

impl<State> MiddlewareStack<State> {
    /// Create an empty stack of middlewares.
    pub fn new() -> MiddlewareStack<State> {
        MiddlewareStack {
            stack: Vec::new(),
        }
    }

    /// Add a middleware to the stack.
    pub fn push(&mut self, m: impl Middleware<State>) {
        self.stack.push(Arc::new(m));
    }
}

impl<State: 'static> Middleware<State> for MiddlewareStack<State> {
    fn handle<'a>(&'a self, cx: Context<State>, next: Next<'a, State>) -> BoxFuture<'a, Response> {
        if let Some((last, others)) = self.stack.split_last() {
            last.handle(cx, Next::new(next.endpoint, others))
        } else {
            next.run(cx)
        }
    }
}

I am not sure if the middleware stack should behave like a stack as the name suggests or like a queue (respectively split_last and split_first). Maybe I can start a pull request and collect some comments/reviews?

edit: I just noted a problem. next should be merged into others instead of being used only into the else branch. I tried something like building a new Next with &[others, next.next_middleware].concat() but obviously the new reference does not live long enough. Also persisting it by adding a field in the MiddlewareStack doesn't seem viable since handle takes an immutable reference.

[CfirTsabari]

As i see it we have three options how to approach this:

1. Add another field To Next next: Option<Arc<Next>> and in the MiddlewareStack' create new Nextthat warp the original Next, and in theNext::run` function call this next if it exist .
2. Improvement to 1 :Change Next to be trait instead of struct, this will allow to create a special Next in `MiddlewareStack' without interfering with the Current Next Implementation. But this might present a problem letting users too much control.
3. Completely inverse the solution and instead of MiddlewareStack' impl Middleware' we should let MiddlewareStack' expose a vec\slice of Middlewarethat will be added to the server(add a function that accept vec\slice ofMiddleware`).

I Tried 1 but got stuck because i am lacking knowledge and experience to work good enough with generic and lifetime.

But after second thought i think that 3 is better.

[pantosaur]

I have been learning the tide code base for a while, and have decided to work on this issue.

Currently I have something working where I added a field called stack of type Vec<&'a [Arc<dyn Middleware<State>>]> to Next, and an extra branch in Next::run. A new slice gets pushed onto stack everytime a MiddlewareStack middleware is popped from the stack, which works with nested MiddlewareStacks. This separates the middlewares added dynamically by a MiddlewareStack and the ones that are part of the Server. I don't think it's possible to keep the same structure for Next without cloning the next_middlewares slice into an owned vector of Arcs, but that could be one option. The other option would be by making Next as a trait as suggested by @CfirTsabari.

I also wrote some tests, and so far it seems to be working fine, and I would welcome some feedback as to whether I'm on the right track.
https://github.com/pantosaur/tide/commit/fee59b8351f42cbc897db9f844f67ba5d828a4d5