pathsend causing issues with BaseHTTPMiddleware

[aersam]

Hi guys

This is a follow up of emmett-framework/granian#320

I'm not entirely sure if this is really a bug of starlette, or more of a feature that's missing or so.

On my staging environment, I did receive an assertion error:

 File "/tmp/8dc89326e92b51b/antenv/lib/python3.12/site-packages/starlette/middleware/base.py", line 173, in body_stream
2024-06-10T09:58:53.052818649Z     assert message["type"] == "http.response.body"

As I could not reproduce it locally, I monkey-patched starlette and printed an error message with details:

2024-06-10T09:58:53.052814649Z   File "/tmp/8dc89326e92b51b/antenv/lib/python3.12/site-packages/starlette/middleware/base.py", line 173, in body_stream
2024-06-10T09:58:53.052818649Z     assert message["type"] == "http.response.body", f"Unexpected message of type {message['type']} from class {type(self)}: {message}"  # noqa: E501
2024-06-10T09:58:53.052821649Z     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-06-10T09:58:53.052824449Z AssertionError: Unexpected message of type http.response.pathsend from class <class 'starlette.middleware.base.BaseHTTPMiddleware'>: {'type': 'http.response.pathsend', 'path': 'frontend_app/output/index.html'}

I think this happens because my server does support the pathsend extension, however middlware that act's on it and just derives from BaseHTTPMiddleware will fail, since it cannot handle this.

Important

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[gi0baro]

This is mitigated by #2649, which is released in 0.38.1.

@Kludex let me know if you prefer to keep this open for the pathsend support general issue, or if I should open up a different issue to track it.

[Kludex]

@gi0baro We are considering deprecating BaseHTTPMiddleware, and removing for 1.0. Only considering for now.

In any case, we don't recommend using BaseHTTPMiddleware for a long time. What we can do is:

1. Add the pathsend support back.
2. Add a conditional on the GzipMiddleware.
3. Recommend Granian users to not use BaseHTTPMiddleware - You can tell people that the Starlette maintainer said that.

Wdyt?

About the GzipMiddleware, I want to avoid adding a conditional every time we have a new extension created, so maybe we can change some conditional (changing an else for a more specific elif?)?

[Kludex]

In any case, I don't want to add more stuff to the BaseHTTPMiddleware, so we can mark this issue as "won't fix".

[gi0baro]

What we can do is:

1. Add the pathsend support back.

Sure, I'll prepare a PR for that.

2. Add a conditional on the GzipMiddleware.
   About the GzipMiddleware, I want to avoid adding a conditional every time we have a new extension created, so maybe we can change some conditional (changing an else for a more specific elif?)?

So instead of an elif like this https://github.com/encode/starlette/pull/2616/files#diff-0457610aa5a32911721d8869dfafdd471bbb89378197914246e291896dd312efR109, you'd prefer an else, did I get it right? If that's the case, makes sense to me.

3. Recommend Granian users to not use BaseHTTPMiddleware - You can tell people that the Starlette maintainer said that.

Maybe we can also add a line in the documentation here (https://www.starlette.io/middleware/#basehttpmiddleware) in the limitations section, wdyt @Kludex?

[Kludex]

Maybe we can also add a line in the documentation here (starlette.io/middleware#basehttpmiddleware) in the limitations section, wdyt @Kludex?

What do you want to add there?

Sure, I'll prepare a PR for that.

👍

So instead of an elif like this #2616 (files), you'd prefer an else, did I get it right? If that's the case, makes sense to me.

I changed my mind. Your elif was fine. Better to be explicit.

[gi0baro]

Maybe we can also add a line in the documentation here (starlette.io/middleware#basehttpmiddleware) in the limitations section, wdyt @Kludex?

What do you want to add there?

I would make it look like this:

Currently, the BaseHTTPMiddleware has some known limitations:

• Using BaseHTTPMiddleware will prevent changes to contextlib.ContextVars from propagating upwards [...]
• Using BaseHTTPMiddleware will prevent ASGI pathsend extension to work properly. Thus, if you run your Starlette application with a server implementing this extension, routes returning FileResponse should avoid the usage of this middleware.