Middleware stacking and recursive initialization

[euri10]

long story short I was writing a logging middleware whose handlers are initialized in the __init__ of the middleware and ended up with multiple logs.

it seems to me the __init__ of the middlewarea are recursively called because of the build_middleware_stack method and I'm not sure if this is intended or a bug.

here's a test case that shows what I think is an issue:

class SimpleInitializableMiddleware:
    def __init__(self, app, counter: int):
        self.app = app
        self.counter = counter
        print(self.counter)


def test_middleware_stack_init():
    app = Starlette()
    app.add_middleware(SimpleInitializableMiddleware, counter=1)
    app.add_middleware(SimpleInitializableMiddleware, counter=2)
    app.add_middleware(SimpleInitializableMiddleware, counter=3)
    app.add_middleware(SimpleInitializableMiddleware, counter=4)

I would expect to see 1 2 3 4 as the output.
however it prints 1 1 2 1 2 3 1 2 3 4 because for each add_middlware call it goes down the stack and reinitialize it.

what you think @tomchristie ?

[thunderamur]

I think it's a bug. It's wrong to make initialization of the same middleware again and again as many times as other middlewares are added.

[thunderamur]

We can use middleware kwarg when init Starlette instead.

app = Starlette(middleware=[
    Middleware(SimpleInitializableMiddleware, counter=1),
    Middleware(SimpleInitializableMiddleware, counter=2),
])

But add_middleware method is incorrect anyway.

[danilito19]

Would a solution here be to deprecate add_middleware and replace it with a add_middlewares method that allows for a list of middlewares to be passed in? That way a user wouldn't have to call add_middleware multiple times and is just expected to call the new add_middlewares once.

[tomchristie]

No. Although I'd query what it actually needs. Presumably it need access to some information on the app, rather than the app itself.

[danilito19]

As mentioned above, the middleware needs the app's router registration to compute a list of routes.

[adriangb]

@danilito19 would #1649 help?

[danilito19]

@adriangb I think so? The main request was that any middleware created with Starlette would have access to the original app and thus the app's router. From my brief understanding of this PR, it looks like that's sort of the case? Can you confirm

[adriangb]

What that PR does is let you insert middleware after routing. If your use case was introspecting into the router so that the middleware is only applied to only some routes, then #1649 solves that by letting you apply that middleware to only those routes. If you want a reference to the app, this will incidentally help you, but not necessarily by design.

[Kludex]

I've created #2002 from this discussion.