#34752 closed Bug (fixed)

ASGI http.disconnect not handled for streaming responses

Reported by: Sam Toyer Owned by: Sam Toyer
Component: HTTP handling Version: dev
Severity: Release blocker Keywords: async
Cc: Dennis Chukwunta, Carlton Gibson, Andrew Godwin Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The fix to #33738 (ticket) correctly handles HTTP disconnects before a view exits (i.e. returns a response object), but does not handle disconnects between the time when the view returns and the time when the response actually finishes. This is a particular problem for long-lived StreamingHttpResponses (e.g. for SSE).

The issue can be solved by extending the original cancellation/disconnect-handling logic to also cover AsgiHandler.send_response(). Example tests and a fix are in this commit: https://github.com/qxcv/django/commit/ed2f9ad03a3abbc636fd5fa590d69ba36571c158

I'm happy to file a PR if this is wanted upstream.

Change History (13)

comment:1 by Mariusz Felisiak, 16 months ago

Cc: Dennis Chukwunta Carlton Gibson added
Severity: NormalRelease blocker

This seems to be a bug in the new feature #33738 (related to the #33735).

comment:2 by Carlton Gibson, 16 months ago

Triage Stage: UnreviewedAccepted

Hey Sam, thanks for the report. Yes, please do open a PR! (It being the weekend, I'd like to have a look at this properly once the PR is open.)

It would be nice to check that the cancelled error was handled explicitly, e.g. in PR for #33738 so similar in the streaming response iterator, and maybe add an example similar to the async views disconnect example to the StreamingHttpResponse docs.

Last edited 16 months ago by Carlton Gibson (previous) (diff)

comment:3 by Carlton Gibson, 16 months ago

Keywords: async added

comment:4 by Carlton Gibson, 16 months ago

Hey Sam, no pressure but can I ask, do you have capacity to submit a PR here? (This is a Release Blocker for Django 5.0, so needs to be resolved before the feature freeze at the beginning of September) Thanks!

comment:5 by Sam Toyer, 15 months ago

Yes, I can do this. There was an error in my original attempt at fixing it, so I've been holding off on submitting a PR until I'm confident the new fix works okay. I should have time to add some more tests and submit the PR tomorrow, though.

comment:6 by Sam Toyer, 15 months ago

Owner: changed from nobody to Sam Toyer
Status: newassigned

comment:7 by Carlton Gibson, 15 months ago

Great. Thanks. 🎁

comment:8 by Sam Toyer, 15 months ago

Filed a PR: https://github.com/django/django/pull/17147

One subtlety: I originally made a smaller modification to the handler that put view logic and response logic in the same coroutine, but otherwise followed the same flow. I found this broke in practice because sometimes both coroutines would return at once. This caused the previous pending.pop()/done.pop() logic to error out because one of those two sets would be empty. I think that this problem is particularly likely to happen in situations where some coroutines are long-lived. I haven't added a test for this edge case yet, though.

comment:9 by Sarah Boyce, 15 months ago

Has patch: set

comment:10 by Mariusz Felisiak, 15 months ago

Patch needs improvement: set

comment:11 by Mariusz Felisiak, 14 months ago

Cc: Andrew Godwin added

comment:12 by Mariusz Felisiak, 14 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 14 months ago

Resolution: fixed
Status: assignedclosed

In 64cea1e:

Fixed #34752 -- Fixed handling ASGI http.disconnect for streaming responses.

Note: See TracTickets for help on using tickets.
Back to Top