allow yielding a new response

[adriangb]

This gives us better API parity with BaseHTTPMiddleware where you get to return a response (the same one or a new one) after you get the response from the wrapped app.

Maybe this could be combined with #1695 to allow this new response to wrap the body of the original response if it wants to? It's unfortunate that users would only be able to return a brand new body and not the existing body.

[florimondmanca]

I think we need to decide a clear stance on what a helper HttpMiddleware should allow doing, and what should be deferred as pure ASGI middleware.

My guiding principle would this: we don’t want to provide an API which might end up with the same issues than we had before. From my POV, anything that touches the request or response body gets us closer to that. Here, an app might return a streaming response: what do we do with it if we replace it with our own response? It’s a slippery slope.

More specifically on this particular PR - what would be a practical motivational example for this kind of usage?

[adriangb]

Great questions! My main reason for making this PR (and tagging you) was the thought that feature parity with BaseHTTPMiddleware would be important. But yes this may not even be a good idea!

[adriangb]

Here, an app might return a streaming response: what do we do with it if we replace it with our own response?

In this implementation we would just discard the streaming generator. Are you worried about garbage collecting it or something specific?

One thing to think about is that this API (returning a new Response) has been around in BaseHTTPMiddleware for a while and as far as I remember it hasn't been broken/problematic (aside from #1022 which was fixed)

[florimondmanca]

Are you worried about garbage collecting it or something specific?

Yes, or rather cleanly closing any appropriate upstream resources that are used to generate the streaming response.

There’s no problem if returning a bounded « standard » response, which contains eg a JSON payload or an HTML page. But if the response is streaming, that means there’s a generator somewhere tied to the more_body interface that might be reading from a file or an external HTTP stream or something, and we can’t have access to that.

AFAIK there’s no way in the ASGI spec to just discard a streaming body even though more_body is True. In fact I believe the spec says servers MUST pull until that becomes False.

Also, I think we do want to treat streaming (eg Transfer Encoding: Chunked) HTTP as first class. It’s so tricky not to shoot oneself in the foot with pending I/O resources that we’ve got an issue open in HTTPX for 3+ years to cleanly add support for it in the ASGITransport…

[adriangb]

Makes sense. If I understand correctly, this should also impact the current implementation of BaseHTTPMiddleware right?

[jhominal]

After thinking on it for a bit, here is my take:

• For either response or request bodies, a middleware can basically either consume the current application (and process ongoing events) or discard its events;
• It seems to me that, for the http scope, the http.disconnect message type would be able to express the semantic of "this ASGIApp object has been disconnected from any useful relationship with the current HTTP request processing", and could be used to trigger finalization logic early in a streaming response (e.g. starlette.responses.StreamingResponse already uses this event to trigger cancellation, while still running background tasks... maybe that could be used as a way of solving Background tasks are cancelled if the client closes connection #1438?);

I know that I am just a simple issue reporter, but I think that before designing the API to produce new responses here, it would be good first to write the following cases of body processing in a pure ASGI middleware (and maybe that has already been done?):

• the middleware consumes and forwards all the http.response.body events (e.g. it wants to log the actual content length of the response, or track the time that the application takes to actually finish processing);
• having called the underlying app, the middleware decides to discard the whole application response and send its own content (e.g. filtering content based on the application headers, like modsecurity does);

[jhominal]

Just so you know, I took a stab at writing pure ASGI middlewares as I previously suggested, here are the results:

• A middleware that simply looks at the request and response bodies

import logging

from starlette.types import ASGIApp, Message, Scope, Receive, Send

logger = logging.getLogger(__name__)


class HttpLoggingMiddleware:
    def __init__(self, app: ASGIApp) -> None:
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        if scope["type"] != "http":
            return await self.app(scope, receive, send)

        request_body_bytes = 0
        response_body_bytes = 0

        async def receive_wrapper() -> Message:
            nonlocal request_body_bytes

            # Get the message from the HTTP layer
            message: Message = await receive()

            # Examine chunks of the HTTP request body that are going through
            if message["type"] == "http.request":
                request_body_bytes += len(message.get("body", b""))
                if not message.get("more_body", False):
                    logger.info(f"Received complete request body of {request_body_bytes} bytes")

            if message["type"] == "http.disconnect":
                logger.warning("Disconnected from the client")

            # Forward the message downwards, to the application
            return message

        async def send_wrapper(message: Message) -> None:
            nonlocal response_body_bytes
            if message["type"] == "http.response.start":
                logger.info(f"Sending response status {message['status']}")

            if message["type"] == "http.response.body":
                response_body_bytes += len(message.get("body", b""))
                if not message.get("more_body", False):
                    logger.info(f"Streamed complete response body of {response_body_bytes} bytes")

            # Send the message towards the HTTP server/client
            await send(message)

        await self.app(scope, receive_wrapper, send_wrapper)

• A middleware that filters the outgoing response and discards it

from starlette.types import ASGIApp, Message, Scope, Receive, Send
import anyio


class HttpFilteringMiddleware:
    def __init__(self, app: ASGIApp) -> None:
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        if scope["type"] != "http":
            return await self.app(scope, receive, send)

        app_disconnected = anyio.Event()

        async def receive_wrapper() -> Message:
            # Application disconnection is already set - immediately send the disconnect event.
            if app_disconnected.is_set():
                return {
                    "type": "http.disconnect"
                }

            message = None
            message_available = anyio.Event()
            async with anyio.create_task_group() as task_group:
                async def set_message_from_upstream():
                    nonlocal message
                    message = await receive()
                    message_available.set()
                async def set_message_on_disconnection():
                    nonlocal message
                    await app_disconnected.wait()
                    message = {"type": "http.disconnect"}
                    message_available.set()

                task_group.start_soon(set_message_from_upstream)
                task_group.start_soon(set_message_on_disconnection)
                await message_available.wait()
                task_group.cancel_scope.cancel()

            # Forward the message downwards, to the application
            return message

        async def send_wrapper(message: Message) -> None:
            if app_disconnected.is_set():
                return

            if message["type"] == "http.response.start":
                if message["status"] == 500:
                    # Send filtered HTTP response
                    await send({
                        "type": "http.response.start",
                        "status": 403,
                        "headers": {
                            (b"Content-Type", b"text/html; charset=utf-8"),
                        }
                    })
                    await send({
                        "type": "http.response.body",
                        "body": b"<h1>Forbidden</h1>",
                        "more_body": False,
                    })

                    # Disconnect downstream application
                    app_disconnected.set()
                    return

            await send(message)

        await self.app(scope, receive_wrapper, send_wrapper)

What do you think?

• Do you agree that, when discarding the response from app, the middleware should send the http.disconnect event?
• Do you think that it would be relevant to add (after review) either of the middleware examples in this comment to Document how to create ASGI middlewares #1656 ?

[adriangb]

The idea has been discussed, this served it's purpose