#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, and maybe add an example similar to the async views disconnect example to the StreamingHttpResponse docs.

Version 0, edited 16 months ago by Carlton Gibson (next)

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, 16 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, 16 months ago

Owner: changed from nobody to Sam Toyer
Status: newassigned

comment:7 by Carlton Gibson, 16 months ago

Great. Thanks. 🎁

comment:8 by Sam Toyer, 16 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, 16 months ago

Has patch: set

comment:10 by Mariusz Felisiak, 15 months ago

Patch needs improvement: set

comment:11 by Mariusz Felisiak, 15 months ago

Cc: Andrew Godwin added

comment:12 by Mariusz Felisiak, 15 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 15 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