Shield send "http.response.start" from cancellation (BaseHTTPMiddleware)

[acjh]

Fixes #1634

• Discussion `RuntimeError` with custom middleware when client drops connection before request finished #1527
• Caused by anyio integration #1157

await recv_stream.receive() will raise anyio.EndOfStream if request is disconnected, due to:

• task_group.cancel_scope.cancel() in StreamingResponse.__call__.<locals>.wrap and
• cancellation check in await checkpoint() of MemoryObjectSendStream.send,

and then RuntimeError: No response returned. will be raised in BaseHTTPMiddleware.

Let's shield send "http.response.start" from cancellation, since the message is ready to be sent to the receiver.

This is an alternative implementation of #1706 in BaseHTTPMiddleware instead of StreamingResponse.
We should not force the shielding in StreamingResponse, since the cancellation check is an intended feature of MemoryObjectSendStream. BaseHTTPMiddleware, which uses both, should be responsible for the compatibility.

[adriangb]

I tried my patch to base.py from #1706 (comment) on this branch and the current test (in this branch, not the patch) also passes (I can confirm the test fails on master so it's not just that it passes on any branch). I do think your solution is nicer in some sense, but does it fix any edge cases that the raise an exception solution doesn't?

[acjh]

I don't think this solution fixes any additional edge cases.

Both solutions pretty much target this specific edge case, where StreamingResponse does listen_for_disconnect and handles the disconnection in a way that is not compatible with how MemorySendStream is used in BaseHTTPMiddleware.

[adriangb]

If there are no pros to doing cancellation, I would prefer to just raise an exception, but that's just my opinion and I'm biased, so I'd like others to weigh in.

Otherwise, this PR looks good and I'd be happy to move forward with it, thank you for the contribution and your persistence @acjh !

[acjh]

If there are no pros to doing cancellation

I believe you mean not doing cancellation. More accurately, it's temporarily shielding from cancellation.

I would prefer to just raise an exception

I'm totally fine with closing this PR in favour of that, btw. 😄

This PR simply handles the compatibility between StreamingResponse and usage of anyio memory streams.
Raising an exception takes control of the flow by forcing exception handling, ignoring the streams' behaviour.

[adriangb]

If there are no pros to doing cancellation

I believe you mean not doing cancellation. More accurately, it's temporarily shielding from cancellation.

Yep sorry for being unclear

I would prefer to just raise an exception

I'm totally fine with closing this PR in favour of that, btw. 😄

This PR simply handles the compatibility between StreamingResponse and usage of anyio memory streams. Raising an exception takes control of the flow by forcing exception handling, ignoring the streams' behaviour.

Let's just wait to see what others think, I'm also fine with either 😄

[acjh]

Sure. Thank you for your patience and prompt responses!

[scottherlihy]

@acjh @adriangb from my perspective I do not think an exception should be thrown because I don't view this as an error condition and I'm curious what your thoughts are. We are encountering this while utilizing a callback url pattern with long running tasks. The client is posting a request, it includes a callback url where to send the response, as such the client disconnects immediately. We have all the information we need to send the response and don't need an active client connection yet this error indeterminately causes dropping of responses via the error documented above. We can certainly catch and handle an error thrown, but I don't think there should be an expectation that the client needs to be connected through the lifetime of the process. Does this make sense or is there something I'm missing?

[acjh]

Are you using background tasks? This PR does not fix #1438.

[scottherlihy]

@acjh we are, I will read more about that other ticket, thank you