Lazily build middleware stack #2017

adriangb · 2023-01-23T14:00:01Z

Closes Middleware stacking and recursive initialization #2002

This is an alternative solution for deprecation + removal of the add_middlware. The solution here is to build the application with its middlewares only when we receive the first ASGI event - lifespan, websocket or http, doesn't matter.

Closes encode#2002

adriangb · 2023-01-23T18:09:12Z

starlette/applications.py

+        if self.middleware_stack is None:
+            self.middleware_stack = self.build_middleware_stack()


Note that this will run when the lifespan is triggered most of the time but if the lifespan is disable it will run on the first request. So not checking scope["type"] is by design.

I've mentioned this in the description as well. 👍

alex-oleshkevich · 2023-01-30T12:31:22Z

starlette/applications.py

@@ -140,16 +141,18 @@ def host(
    def add_middleware(
        self, middleware_class: type, **options: typing.Any
    ) -> None:  # pragma: no cover
+        if self.middleware_stack is not None:


This method is deprecated, isn't it @Kludex ?

Yes it is. #2002 still came up last meeting as a problem.

It's not...

We didn't deprecate it. We only deprecated the decorators. 🤔

And "what came up on the meeting" was that I mentioned the add_middleware as an issue, due to the linked GitHub issue above, and the idea was to deprecate, and further remove, but Adrian came up with this alternative solution.

Apologies, thank you for clarifying 😄

alex-oleshkevich · 2023-01-30T12:32:30Z

starlette/applications.py

@@ -74,7 +74,7 @@ def __init__(
            {} if exception_handlers is None else dict(exception_handlers)
        )
        self.user_middleware = [] if middleware is None else list(middleware)
-        self.middleware_stack = self.build_middleware_stack()
+        self.middleware_stack: typing.Optional[ASGIApp] = None


IMO, build_middleware_stack is a very cheap function to call. It makes sense to call it here and call once again when you need it again, and avoid nullable attributes (and null-checks).

Moreover, it is not expected to be called often at runtime and affects only startup (configuration) time.
So calling it multiple times seems ok to me.

I’m not sure if you advocating for the old existing approach with the one being proposed here. Performance is not the goal, it is to avoid instantiating users middleware repeatedly as reported in #2002

starlette/applications.py

Kludex · 2023-02-04T09:27:37Z

starlette/applications.py

@@ -140,16 +141,18 @@ def host(
    def add_middleware(
        self, middleware_class: type, **options: typing.Any
    ) -> None:  # pragma: no cover
+        if self.middleware_stack is not None:


It's not...

We didn't deprecate it. We only deprecated the decorators. 🤔

And "what came up on the meeting" was that I mentioned the add_middleware as an issue, due to the linked GitHub issue above, and the idea was to deprecate, and further remove, but Adrian came up with this alternative solution.

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

starlette/applications.py

Kludex · 2023-02-04T09:42:02Z

I think Flask 2.0 introduced a similar idea... I can't recall how was it. Someone knows?

Kludex · 2023-02-04T09:44:58Z

I think there's no need to add anything to the documentation regarding this change.

This change shouldn't impact anyone - only if they are relying on the multiple instantiations' behavior, which I doubt.

Maybe we can test this against FastAPI to see if there's something that may not be obvious that breaks over there - I can do it before the next release. 👍

Kludex · 2023-02-06T05:33:06Z

starlette/applications.py

-        self, middleware_class: type, **options: typing.Any
-    ) -> None:  # pragma: no cover
+    def add_middleware(self, middleware_class: type, **options: typing.Any) -> None:
+        if self.middleware_stack is not None:  # pragma: no cover


Can't we add a test for this case? 🤔

Seems like testing for the sake of testing but if you want I can add it.

Kludex · 2023-02-06T05:34:57Z

tests/test_applications.py

@@ -486,3 +489,45 @@ async def startup():

        app.on_event("startup")(startup)
        assert len(record) == 1
+
+
+def test_middleware_stack_init(test_client_factory: Callable[[ASGIApp], httpx.Client]):


This would be the first time using httpx.Client on this annotation on the test suite 👀

Suggested change

def test_middleware_stack_init(test_client_factory: Callable[[ASGIApp], httpx.Client]):

def test_middleware_stack_init(test_client_factory: Callable[[ASGIApp], TestClient]):

Hmm yeah I should have used TestClient. Luckily it's just a type annotation.

Kludex · 2023-02-06T05:36:19Z

tests/test_applications.py

+    app = get_app()
+
+    test_client_factory(app).get("/foo")
+
+    assert SimpleInitializableMiddleware.counter == 2


How relevant is this part for the test? 🤔

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Mark90 · 2023-03-29T12:45:22Z

@Kludex

This change shouldn't impact anyone - only if they are relying on the multiple instantiations' behavior, which I doubt.

I don't know if that's what we're doing. But I did just notice this in one of our FastAPI projects and reduced it to the following example:

# main.py
from fastapi import FastAPI
from fastapi.requests import *
from fastapi.responses import *

class MyHandledException(Exception):
    """My handled exception."""

async def my_exception_handler(
    request: Request, exc: MyHandledException
) -> PlainTextResponse:
    # fastapi==0.90.1 starlette==0.23.1   -> my_exception_handler executes
    # fastapi==0.91.0 starlette==0.24.0   -> my_exception_handler does not execute
    print(">>> Executing my_exception_handler <<<")
    return PlainTextResponse(
        f"Caught the following exception: {str(exc)}", status_code=400
    )

app = FastAPI()

@app.on_event("startup")
async def on_startup() -> None:
    print(">>> Executing on_startup <<<")
    app.add_exception_handler(MyHandledException, my_exception_handler)

@app.get("/{thing}")
async def get_thing(thing):
    if thing == "boom":
        raise MyHandledException("Boom")
    return f"Here you go: {thing}"

In a Python 3.11 venv with uvicorn==0.21.1 fastapi==0.90.1 starlette==0.23.1, started the app with uvicorn main:app, called the endpoint with "/boom" -> MyHandledException is picked up by the exception handler just fine.

After upgrading to fastapi==0.91.0 starlette=0.24.0 -> the on_startup is executed, but MyHandledException is not picked up by the handler. It doesn't seem to be in the middleware stack, I'm not completely sure why. Perhaps the stack is already initialized at this point?

When I register the handler like recommended in the FastAPI docs as below, then it does work

@app.execution_handler(MyHandledException)
async def my_exception_handler(
...

In our real application the exception handler comes from another package, so adding the @app.exception_handler decorator is not an option.

But I've also read somewhere that on_startup event is going to be replaced by "Lifespan", I'll have to read up on that (to see if it can be used for dynamically registering exception handlers)

Kludex · 2023-03-29T13:16:56Z

Perhaps the stack is already initialized at this point?

Yes. It's the middleware stack.

Note: I don't think we predicted users doing this @adriangb... 🤔

adriangb · 2023-03-29T13:21:02Z

Oh jeez, we certainly did not predict this. Technically with the previous implementation you could add middleware at any time, even within a running request/response cycle, which I don't think should be allowed, period. Adding it within the lifespan seams a bit more reasonable: you could have a middleware that depends on something you initialize in the lifespan.
The good news is that we can probably move building the stack until after the ASGI lifespan has been run, which would solve the example above.

Kludex · 2023-03-29T13:53:27Z

starlette/applications.py

+        if self.middleware_stack is None:
+            self.middleware_stack = self.build_middleware_stack()


Suggested change

if self.middleware_stack is None:

self.middleware_stack = self.build_middleware_stack()

if scope["type"] != "lifespan" and self.middleware_stack is None:

self.middleware_stack = self.build_middleware_stack()

What if the middleware runs something on lifespan? Maybe we need to build once before running the lifespan and then, if it's changed, we re-build it after running the lifespan?

Actually we want both scenarios...

What do you mean?

What do you mean?

The same thing as your first message.

But actually... We don't want to rebuild, otherwise we introduce the issue that motivated this PR again.

I think I have a solution that allows dynamically adding middleware at any point in time and never re-building:

class ASGIAppProxy: def __init__(self, app: ASGIApp) -> None: self.app = app async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: await self.app(scope, receive, send)

The reason we need to re-build in the first place is that the last user middleware needs to point to self.router/ExceptionMiddleware. So say you have a single A user middelware, the "stack" looks like ServerErrorMiddleware -> UserMiddlewareA -> ExceptionMiddleware -> Router. Then you add a UserMiddlewareB and it should be ServerErrorMiddleware -> UserMiddlewareA -> UserMiddlewareB -> ExceptionMiddleware -> Router. So you need to change the ASGIApp that UserMiddlewareA is pointing to from ExceptionMiddleware to UserMiddlewareB. We can't just assign a .app attribute on the users middleware because it could be anything. By introducing the above wrapper we can do something like:

exc_middleware = ExceptionMiddleware(...) tail = ASGIAppProxy(exc_middleware) user_middleware_tail = UserMiddlewareA(tail) # add UserMiddlewareB new_tail = ASGIAppProxy(exc_middleware) new_user_middleware_tail = UserMiddlewareB(new_tail) tail.app = new_user_middleware_tail tail = new_tail

Superseeds encode#2017 and fixes encode#2017 (comment)

Kludex · 2023-03-30T12:40:48Z

...you could have a middleware that depends on something you initialize in the lifespan.

For example? You can (and I guess you should) use the scope["state"] in that middleware.

@Mark90 can you share what's the exact use-case that you have?

Kludex · 2023-03-30T12:43:46Z

We can also add an error on add_middleware in case it's used after the middleware stack is built.

adriangb · 2023-03-30T12:44:52Z

We can also add an error on add_middleware in case it's used after the middleware stack is built.

We already have one: https://github.com/encode/starlette/blob/master/starlette/applications.py#L139

adriangb · 2023-03-30T12:51:31Z

For example?

An authentication middleware that needs a database connection or HTTP client.

You can (and I guess you should) use the scope["state"] in that middleware.

I had not thought of that use case for scope[“state”], but it’s a good idea. Even without that feature of the middleware could just store whatever it needs on its instance.

Mark90 · 2023-03-30T12:51:35Z

@Kludex

@Mark90 can you share what's the exact use-case that you have?

We have a library containing endpoints / exceptions (and handlers) that we reuse among a few projects. Using the shared exception handler in those projects requires .add_exception_handler() as far as I know.

adriangb · 2023-03-30T13:15:51Z

Why can’t you pass those to the constructor or call add_exception_handler before the lifespan?

Mark90 · 2023-03-30T14:55:57Z

Ah, I only checked FastAPI's docs for exception handlers, not Starlette's. Didn't know about that constructor parameter, it works perfect. Thank you!

adriangb added 4 commits January 23, 2023 07:50

Lazily build middleware

d952ac2

Closes encode#2002

add test

e09fd5d

add test

8654547

fix types

4edcb80

adriangb commented Jan 23, 2023

View reviewed changes

adriangb requested review from Kludex and a team January 23, 2023 18:09

alex-oleshkevich reviewed Jan 30, 2023

View reviewed changes

Merge branch 'master' into fix-add-middleware

13fee72

Kludex reviewed Feb 4, 2023

View reviewed changes

Update starlette/applications.py

0816c33

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Kludex reviewed Feb 4, 2023

View reviewed changes

starlette/applications.py Outdated Show resolved Hide resolved

Kludex mentioned this pull request Feb 4, 2023

Middleware stacking and recursive initialization #2002

Closed

adriangb added 4 commits February 4, 2023 17:32

Merge branch 'master' into fix-add-middleware

328c1a3

lint

6fd9d57

skip coverage on error

fd0507e

remove setter

9a1a15b

Kludex approved these changes Feb 6, 2023

View reviewed changes

Merge branch 'master' into fix-add-middleware

bc154fd

adriangb enabled auto-merge (squash) February 6, 2023 05:34

Kludex reviewed Feb 6, 2023

View reviewed changes

adriangb merged commit 51c1de1 into encode:master Feb 6, 2023

adriangb deleted the fix-add-middleware branch February 6, 2023 05:35

Kludex reviewed Feb 6, 2023

View reviewed changes

Kludex mentioned this pull request Feb 10, 2023

Use .debug instead of ._debug on FastAPI fastapi/fastapi#5984

Closed

aminalaee pushed a commit that referenced this pull request Feb 13, 2023

Lazily build middleware stack (#2017)

3ae673b

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

frankie567 mentioned this pull request Mar 7, 2023

Test test_expose_defaults fails due to NameError after updating trallnag/prometheus-fastapi-instrumentator#219

Closed

brent-moffit mentioned this pull request Mar 20, 2023

Warning triggered by asgi.OpenTelemetryMiddleware open-telemetry/opentelemetry-python-contrib#1335

Open

Kludex reviewed Mar 29, 2023

View reviewed changes

adriangb added a commit to adriangb/starlette that referenced this pull request Mar 30, 2023

Use a proxy to modify middleware chain in place

453cb8e

Superseeds encode#2017 and fixes encode#2017 (comment)

adriangb mentioned this pull request Mar 30, 2023

Use a proxy object to modify middleware chain in place #2100

Closed

sync-by-unito bot mentioned this pull request Aug 28, 2023

Bump starlette from 0.22.0 to 0.25.0 toddbirchard/ghost-webhook-api#294

Closed

maxkomarychev mentioned this pull request Nov 6, 2023

FastAPI uninstrument disallows adding more middlewares with fastapi@>=0.91 and starlette@>=0.24 open-telemetry/opentelemetry-python-contrib#2042

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazily build middleware stack #2017

Lazily build middleware stack #2017

adriangb commented Jan 23, 2023 •

edited by Kludex

Loading

adriangb Jan 23, 2023 •

edited

Loading

Kludex Feb 4, 2023

alex-oleshkevich Jan 30, 2023

adriangb Jan 30, 2023

Kludex Feb 4, 2023

adriangb Feb 4, 2023

alex-oleshkevich Jan 30, 2023

alex-oleshkevich Jan 30, 2023

adriangb Jan 30, 2023 •

edited

Loading

Kludex Feb 4, 2023

Kludex commented Feb 4, 2023

Kludex commented Feb 4, 2023 •

edited

Loading

Kludex Feb 6, 2023

adriangb Feb 6, 2023

Kludex Feb 6, 2023

adriangb Feb 6, 2023

Kludex Feb 6, 2023

Mark90 commented Mar 29, 2023 •

edited

Loading

Kludex commented Mar 29, 2023 •

edited

Loading

adriangb commented Mar 29, 2023

Kludex Mar 29, 2023

adriangb Mar 29, 2023

Kludex Mar 29, 2023

adriangb Mar 29, 2023

Kludex Mar 29, 2023

Kludex Mar 30, 2023

adriangb Mar 30, 2023

Kludex commented Mar 30, 2023 •

edited

Loading

Kludex commented Mar 30, 2023

adriangb commented Mar 30, 2023

adriangb commented Mar 30, 2023 •

edited

Loading

Mark90 commented Mar 30, 2023

adriangb commented Mar 30, 2023

Mark90 commented Mar 30, 2023

		if self.middleware_stack is None:
		self.middleware_stack = self.build_middleware_stack()

	def test_middleware_stack_init(test_client_factory: Callable[[ASGIApp], httpx.Client]):
	def test_middleware_stack_init(test_client_factory: Callable[[ASGIApp], TestClient]):

Lazily build middleware stack #2017

Lazily build middleware stack #2017

Conversation

adriangb commented Jan 23, 2023 • edited by Kludex Loading

adriangb Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex commented Feb 4, 2023

Kludex commented Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark90 commented Mar 29, 2023 • edited Loading

Kludex commented Mar 29, 2023 • edited Loading

adriangb commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex commented Mar 30, 2023 • edited Loading

Kludex commented Mar 30, 2023

adriangb commented Mar 30, 2023

adriangb commented Mar 30, 2023 • edited Loading

Mark90 commented Mar 30, 2023

adriangb commented Mar 30, 2023

Mark90 commented Mar 30, 2023

adriangb commented Jan 23, 2023 •

edited by Kludex

Loading

adriangb Jan 23, 2023 •

edited

Loading

adriangb Jan 30, 2023 •

edited

Loading

Kludex commented Feb 4, 2023 •

edited

Loading

Mark90 commented Mar 29, 2023 •

edited

Loading

Kludex commented Mar 29, 2023 •

edited

Loading

Kludex commented Mar 30, 2023 •

edited

Loading

adriangb commented Mar 30, 2023 •

edited

Loading