Opened 2 years ago

Closed 21 months ago

#18796 closed Cleanup/optimization (fixed)

Refactor HttpResponse.__iter__ / _get_content

Reported by: aaugustin Owned by: aaugustin
Component: HTTP handling Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by aaugustin)

The current implementation isn't DRY. It seems to me that _get_content should just return b''.join(self).

Unfortunately, implementing this naively results in test failures.

This code is sensitive, tricky, and lacks comment. A refactoring would be useful.

See also #4969, #16494, #18764, [5f2d9cdbb1], [2892cb0ec4], and [536b030363].

To sum up: this ticket is about cleaning up the conversion of HTTP response content to bytes.

Attachments (1)

18796.patch (4.8 KB) - added by aaugustin 21 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 21 months ago by aaugustin

When HttpResponse is instantiated with an iterator, the contents of the iterator should be consumed and saved as soon as request.content is accessed.

It remains to be seen if we want to keep the current (undocumented) streaming behavior of HttpResponse.

This was initially part of #7581 — but I'm reducing the scope of that ticket to something manageable.

Version 0, edited 21 months ago by aaugustin (next)

comment:2 Changed 21 months ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:3 Changed 21 months ago by aaugustin

This comment was made on #7581 but it actually falls in the scope of this ticket:

Returning self._iterator directly in HttpStreamingResponse.streaming_content, instead of a new iterator that yields bytestrings means that middleware authors can't expect consistent bytestring chunks when wrapping streaming_content in a new iterator? They may have to type check or coerce each chunk (potentially reproducing something like make_bytes()? For comparison, HttpResponse.content always returns a bytestring.

comment:4 Changed 21 months ago by aaugustin

  • Description modified (diff)

comment:5 Changed 21 months ago by aaugustin

See also #16494 and #10190.

Changed 21 months ago by aaugustin

comment:6 Changed 21 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In da56e1bac6449daef9aeab8d076d2594d9fd5b44:

Fixed #18796 -- Refactored conversion to bytes in HttpResponse

Thanks mrmachine for the review.

Add Comment

Modify Ticket

Change Properties
<Author field>
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.