Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#34342 closed Bug (fixed)

The test async_client is not consuming async StreamingResponse generators properly

Reported by: Alexandre Spaeth Owned by: Alexandre Spaeth
Component: Testing framework Version: 4.2
Severity: Release blocker Keywords: async
Cc: 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

It seems that the way the async_client behaves with StreamingResponse makes it not play nicely with the fixes done in 4.2 to support async generators.

I think the issue is in closing_iterator_wrapper: we are naively calling sync_to_async on it, but I don’t believe this is possible with a proper async generator.

Here I tried to write a test to reproduce the issue:
https://github.com/Alexerson/django/commit/b05e524bfadfb0d553438d0375bad0009439fade

Looking at the codebase, I’m also believing that there are other parts that might break, for instance assertContains seems to expect a well behaved sync generator.

Change History (8)

comment:1 by Alexandre Spaeth, 21 months ago

I pushed my observations I tiny bit further and I think something as simple as this is a proper fix for the issue:
https://github.com/django/django/pull/16559

Also, I confirm the issue for assertContains, but it could be harder to fix, because we will need an async version of assertContains just for this, and this is probably overkill. In a case like that, some documentations might be enough to solve the issue.

Last edited 21 months ago by Alexandre Spaeth (previous) (diff)

comment:2 by Carlton Gibson, 21 months ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for the report. Eyeballing it looks correct. I will investigate this morning.

Bug in 0bd2c0c9015b53c41394a1c0989afbfd94dc2830

I’m also believing that there are other parts that might break, for instance assertContains seems to expect a well behaved sync generator.

Yes, maybe. There are a few places where the testing assertion and context managers aren't async compatible (yet). We can address those separately.

comment:3 by Carlton Gibson, 21 months ago

Has patch: set
Owner: changed from nobody to Alexandre Spaeth
Status: newassigned

comment:4 by Carlton Gibson, 21 months ago

Triage Stage: AcceptedReady for checkin

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

In bfb8fda:

Refs #34342 -- Added tests for handling sync streaming responses by test client.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

Resolution: fixed
Status: assignedclosed

In 52b05482:

Fixed #34342, Refs #33735 -- Fixed test client handling of async streaming responses.

Bug in 0bd2c0c9015b53c41394a1c0989afbfd94dc2830.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

In 1ecbc04:

[4.2.x] Refs #34342 -- Added tests for handling sync streaming responses by test client.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

Backport of bfb8fda3e69cc6f5c6695ba70117faff51cc25a9 from main

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

In 610cd06c:

[4.2.x] Fixed #34342, Refs #33735 -- Fixed test client handling of async streaming responses.

Bug in 0bd2c0c9015b53c41394a1c0989afbfd94dc2830.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

Backport of 52b054824e899db40ba48f908a9a00dadc56cb89 from main

Note: See TracTickets for help on using tickets.
Back to Top