Regression in v3.8.0

[carltongibson]

Regression in v3.8.0

[ttys0dev]

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]

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.

[carltongibson]

The task is cancelled. It's the communicator that times out waiting for the response.

[ttys0dev]

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?

[carltongibson]

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.

[carltongibson]

@ttys0dev Thanks for taking a look. 🎁

[ttys0dev]

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.

[carltongibson]

@ttys0dev great thanks. I will give it a run in the morning.

Good hustle!

[carltongibson]

https://pypi.org/project/asgiref/3.8.1/