Don't poll for disconnects in BaseHTTPMiddleware via StreamingResponse

[adriangb]

Fixes #2516

[mikkelduif]

I can confirm that this PR fixes the example I have reproduced in #2516 (comment)

[mikkelduif]

I do still recommend you avoid BaseHTTPMiddleware

Maybe we should consider to add a section to: https://www.starlette.io/middleware/#basehttpmiddleware emphasising this.
I do not know the exact technicalities for this, however, it might help future users to avoid it.

[adriangb]

Seems like the issue now is coverage in tests (all of the tests still pass). There are several pieces of test code that are not not being hit but I'm not sure if they were essential or just an implementation detail, some more careful evaluation is needed.

[avihaiyosef]

@adriangb, what is the status of this PR?

[dlahyani]

Hi @adriangb , thanks for this fix.
Any expectation on when this is going to land in a new release of starlette?

[Kludex]

This PR needs someone motivated to check why the coverage dropped.

Anyone that uses BaseHTTPMiddleware in their company, and wants to spend some engineers hours working on this, is welcome.

Otherwise, you can try to sponsor @adriangb as an appreciation gesture for his efforts here, and maybe motivate him to continue the work here.

[isidentical]

@Kludex thanks for the reminder to sponsor, @adriangb has been doing amazing work and we just hit an issue that might be related to this PR so we just started sponsoring (albeit modestly) thru github!!! Would highly recommend anyone else to do the same.

[adriangb]

@Kludex I looked into the coverage.

Basically before we were using StreamingResponse which calls receive() to check for disconnection.
Since I changed this to use a similar implementation but one that does not check for disconnection receive() is no longer called.

For example, if I check out master and apply the changes to test_base.py I get the following traceback:

tests/middleware/test_base.py:776: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
starlette/middleware/base.py:189: in __call__
    with collapse_excgroups():
../../.pyenv/versions/3.12.2/lib/python3.12/contextlib.py:158: in __exit__
    self.gen.throw(value)
starlette/_utils.py:89: in collapse_excgroups
    raise exc
starlette/responses.py:265: in __call__
    await wrap(partial(self.listen_for_disconnect, receive))
starlette/responses.py:261: in wrap
    await func()
starlette/responses.py:238: in listen_for_disconnect
    message = await receive()
starlette/middleware/base.py:54: in wrapped_receive
    msg = await self.receive()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    async def rcv() -> AsyncGenerator[Message, None]:
        yield {"type": "http.request", "body": b"1", "more_body": True}
        yield {"type": "http.request", "body": b"2", "more_body": True}
        yield {"type": "http.request", "body": b"3"}
>       raise AssertionError(  # pragma: no cover
            "Should not be called, no need to poll for disconnect"
        )
E       AssertionError: Should not be called, no need to poll for disconnect

[adriangb]

@Kludex thanks for the reminder to sponsor, @adriangb has been doing amazing work and we just hit an issue that might be related to this PR so we just started sponsoring (albeit modestly) thru github!!! Would highly recommend anyone else to do the same.

Thank you for your sponsorship!

[hakanutku]

@Kludex Could you please review this PR when you get a chance? I appreciate your time!

[Kludex]

I'm a bit tired of people asking me stuff.

I'm very dedicated to the projects I maintain, which means people just need to wait a bit. I don't like people pressuring me to review stuff.

You want to motivate me to check this PR? Feel free to sponsor me. Otherwise, wait for me to voluntarily come here and review it.

Too many issues regarding BaseHTTPMiddleware, and I need to check previous work and make sure this is fine. It takes time.

[dzhulgakov]

is the removal of background here intentional? We use background for both actual background tasks but also to detect when a streaming request was cancelled by the client (request.is_disconnected had some minor overhead).

[dmitry-mli]

Cutting a PR for this

[dmitry-mli]

#2687