Opened 16 months ago
Closed 14 months ago
#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 StreamingHttpResponse
s (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 , 16 months ago
Cc: | added |
---|---|
Severity: | Normal → Release blocker |
comment:2 by , 16 months ago
Triage Stage: | Unreviewed → Accepted |
---|
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.
comment:3 by , 16 months ago
Keywords: | async added |
---|
comment:4 by , 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 , 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 , 15 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 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 , 15 months ago
Has patch: | set |
---|
comment:10 by , 15 months ago
Patch needs improvement: | set |
---|
comment:11 by , 14 months ago
Cc: | added |
---|
comment:12 by , 14 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
This seems to be a bug in the new feature #33738 (related to the #33735).