Closed carltongibson closed 7 months ago
I think this may be a buggy test:
Sending a http.disconnect
AFAIU is supposed to trigger a task cancellation exception not a asyncio.TimeoutError
.
The task is cancelled. It's the communicator that times out waiting for the response.
It's the communicator that times out waiting for the response.
Hmm, but the communicator only waits if the task is not already done right? And the http.disconnect
is already called before await communicator.receive_output()
so wouldn't it be expected to not hit the codepath which would raise a asyncio.TimeoutError
since there would already be an asyncio.CancelledError
?
ASGIHandler runs two parallel tasks. One to process the request, the other to listen for the http.disconnect
event.
In the test here, on receiving the http.disconnect
that task completes, and the other task, processing the request should be cancelled.
When the test fails — and it's intermittent so the behaviour here isn't deterministic — the main request processing task isn't cancelled, which can be checked by inspecting the output received by the application communicator.
That's a regression. We need to work out why it's happened, then we can resolve it. In the meantime though, unless we can solve it quickly, I think we need to revert the problem patch and issue a 3.8.1 to avoid a breakage in deployed applications.
That's a regression. We need to work out why it's happened, then we can resolve it. In the meantime though, unless we can solve it quickly, I think we need to revert the problem patch and issue a 3.8.1 to avoid a breakage in deployed applications.
Debugging it now.
@ttys0dev Thanks for taking a look. 🎁
When the test fails — and it's intermittent so the behaviour here isn't deterministic — the main request processing task isn't cancelled, which can be checked by inspecting the output received by the application communicator.
Yeah, so it looks like the issue was that we were effectively inhibiting normal parent task asyncio.CancelledError
propagation if the parent task was already done by the time we finished cancelling child tasks, #455 should fix that by checking if the task is done and propagating the asyncio.CancelledError
if it was.
@ttys0dev great thanks. I will give it a run in the morning.
Good hustle!
The recent 3.8.0 release causes an intermittent test failure on Django's test suite.
https://djangoci.com/job/pull-requests-focal/database=spatialite,label=focal-pr,python=python3.11/24499/testReport/junit/asgi.tests/ASGITest/test_disconnect_with_body/
This tests disconnect handling for a request with a body. The main task processing the request is not correctly cancelled, on some runs only. (The assertion fails, with response content
{'type': 'http.response.start', 'status': 200, 'headers': [(b'Content-Type', b'text/html; charset=utf-8'), (b'Content-Length', b'12')]}
delivered, rather than the timeout.)Bisected to 0503c2c4df06dbe56f04689fe537325f28dbbe8b by @ttys0dev for #435.
//cc @felixxm