[Django] #34752: ASGI http.disconnect not handled for streaming responses

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
-----------------------------------------+------------------------
Reporter: Sam Toyer | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34752>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#34752: ASGI http.disconnect not handled for streaming responses

---------------------------------+--------------------------------------

Reporter: Sam Toyer | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: dev

Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

---------------------------------+--------------------------------------
Changes (by Mariusz Felisiak):

* cc: Dennis Chukwunta, Carlton Gibson (added)
* severity: Normal => Release blocker

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:1>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+------------------------------------

Reporter: Sam Toyer | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------

Changes (by Carlton Gibson):

* stage: Unreviewed => Accepted

Comment:

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,
[https://github.com/django/django/pull/16603/files#diff-
5f36804bb607499371385c6e3bbf30565150688c425d94ab0527af8af96a4f66R355 e.g.
in PR for #33738], and maybe add an example similar to the
[https://docs.djangoproject.com/en/dev/topics/async/#handling-disconnects
async views disconnect example] to the
[https://docs.djangoproject.com/en/dev/ref/request-response
/#streaminghttpresponse-objects StreamingHttpResponse docs].

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:2>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+------------------------------------
Reporter: Sam Toyer | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:

Keywords: async | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Changes (by Carlton Gibson):

* keywords: => async

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:3>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+------------------------------------
Reporter: Sam Toyer | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------

Comment (by Carlton Gibson):

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!

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:4>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+------------------------------------
Reporter: Sam Toyer | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------

Comment (by Sam Toyer):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:5>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses

---------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned

Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

---------------------------------+-------------------------------------
Changes (by Sam Toyer):

* owner: nobody => Sam Toyer
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:6>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-------------------------------------

Comment (by Carlton Gibson):

Great. Thanks. 🎁

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:7>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-------------------------------------

Comment (by Sam Toyer):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:8>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:9>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
---------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:10>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* cc: andrewgodwn (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:11>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
---------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* cc: Andrew Godwin (added)

[Django]

#34752: ASGI http.disconnect not handled for streaming responses

-------------------------------------+-------------------------------------

Reporter: Sam Toyer | Owner: Sam Toyer
Type: Bug | Status: assigned
Component: HTTP handling | Version: dev
Severity: Release blocker | Resolution:

Keywords: async | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:12>

[Django]

#34752: ASGI http.disconnect not handled for streaming responses
-------------------------------------+-------------------------------------
Reporter: Sam Toyer | Owner: Sam Toyer

Type: Bug | Status: closed

Component: HTTP handling | Version: dev

Severity: Release blocker | Resolution: fixed

Keywords: async | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"64cea1e48f285ea2162c669208d95188b32bbc82" 64cea1e]:
{{{
#!CommitTicketReference repository=""
revision="64cea1e48f285ea2162c669208d95188b32bbc82"
Fixed #34752 -- Fixed handling ASGI http.disconnect for streaming
responses.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/34752#comment:13>