Standardize HTTPResponse.read(X) behavior regardless of compression #2798

pquentin · 2022-11-11T07:46:40Z

Using the findings from #2787, I reimplemented #2712 to only do one copy at worst, using 4GiB of memory for a 2GiB read. This is much better than #2712 which was doing three copies at worst, and two copies when using memory view.

More importantly, this is equivalent to the current solution where we also have one copy at worst when calling self._decode()! So the blockers from #2718 are gone.

Closes #709 (given the decision made in #2769), Closes #2712, Closes #2128

Open questions:

Do we flush the decoder when reaching EOF in partial reads? #2799
Should we prevent read(decode_content=True) followed by read(decode_content=False)? #2800
should we avoid using a buffer when the content is not encoded?
should we try to do bigger read() at the beginning if amt is really small? or save it for later as this isn't a regression

Co-authored-by: Franek Magiera <framagie@gmail.com>

Co-authored-by: Franek Magiera <framagie@gmail.com> When asking for amt bytes, we'll always return that amount unless there is not enough data left. When no data is left, we return b'' to signal EOF. This is the only situation where we can return an empty number of bytes.

The buffer was "losing" in test_requesting_large_resources_via_ssl, but only because there was no compression.

sethmlarson

This looks great, only had a few nits and two question then we can ship!

sethmlarson · 2022-11-11T15:20:31Z

src/urllib3/response.py

+        while fetched < n:
+            remaining = n - fetched
+            chunk = self.buffer.popleft()
+            if remaining < len(chunk):


len(chunk) is calculated 3 times, we can cache this value.

I usually don't add micro optimizations without benchmarking, but this time I was lazy: b24d1da (#2798)

sethmlarson · 2022-11-11T15:21:08Z

src/urllib3/response.py

+                left_chunk, right_chunk = chunk[:remaining], chunk[remaining:]
+                ret.write(left_chunk)
+                self.buffer.appendleft(right_chunk)
+                self._size -= len(left_chunk)


If I'm reading the logic right, len(left_chunk) will always be "remaining", right?

Addressed in b24d1da (#2798)

src/urllib3/response.py

sethmlarson · 2022-11-11T15:31:38Z

src/urllib3/response.py

+
+    def get(self, n: int) -> bytes:
+        if not self.buffer:
+            raise ValueError("buffer is empty")


nit: Should this be a RuntimeError instead of ValueError?

Addressed in 0dc26a3 (#2798).

sethmlarson · 2022-11-11T15:35:02Z

src/urllib3/response.py


-            data = self._decode(data, decode_content, flush_decoder)
+        if not data and len(self._decoded_buffer) == 0:


This is one of the locations you're referencing for #2799 ?

Yes. More precisely, #2799 is an issue since urllib3 1.11, and this is a new place that is wrong and will need to be updated if we decide to fix #2799.

test/test_response.py

pquentin · 2022-11-12T19:07:45Z

My implementation of #2800 breaks the requests test suite, not sure why yet.

sethmlarson · 2022-11-12T19:12:46Z

@pquentin I wonder if we need to change the call to read() with decode_content=False inside of the "drain" method on HTTPResponse? I know Requests allows for raw data access.

This reverts commit f894e83 and 561fb47.

sethmlarson

This looks excellent, let's merge!

The deadsnakes PPA no longer supports 3.11-dev it seems.

pquentin requested review from sethmlarson and shazow as code owners November 11, 2022 07:46

pquentin and others added 3 commits November 11, 2022 11:47

Remove upstreamed botocore patch

07c4ab8

Extract _raw_read into its own function

c311087

Co-authored-by: Franek Magiera <framagie@gmail.com>

Reduce indentation

34ff983

pquentin force-pushed the extract-raw-read branch from 077d3b3 to 34ff983 Compare November 11, 2022 07:49

pquentin added 2 commits November 11, 2022 11:54

Improve cache_content tests

7318ba6

Add changelog

8694e31

pquentin force-pushed the extract-raw-read branch from ee8fe37 to 9e83b03 Compare November 11, 2022 09:25

pquentin mentioned this pull request Nov 11, 2022

Should we prevent read(decode_content=True) followed by read(decode_content=False)? #2800

Closed

3 tasks

Add BytesQueueBuffer implementation

5ad21c6

pquentin force-pushed the extract-raw-read branch from 9e83b03 to 75735fe Compare November 11, 2022 12:27

pquentin added 2 commits November 11, 2022 16:28

Fix BytesQueueBuffer types

a7933eb

pquentin force-pushed the extract-raw-read branch from 75735fe to a7933eb Compare November 11, 2022 12:29

sethmlarson mentioned this pull request Nov 11, 2022

Receiving segmentation fault when testing on GitHub Actions + Linux bloomberg/memray#249

Closed

1 task

pquentin force-pushed the extract-raw-read branch from 4f9d876 to 9674b7a Compare November 11, 2022 15:07

pquentin added 2 commits November 11, 2022 19:37

Test HTTPResponse.read() buffer with compression

d439923

The buffer was "losing" in test_requesting_large_resources_via_ssl, but only because there was no compression.

Add TODO

682beec

pquentin force-pushed the extract-raw-read branch from e272839 to 9f2e2a9 Compare November 11, 2022 15:38

sethmlarson previously approved these changes Nov 11, 2022

View reviewed changes

Install pytest-memray conditionally

c9cf398

pquentin dismissed sethmlarson’s stale review via c9cf398 November 11, 2022 15:43

pquentin force-pushed the extract-raw-read branch from 9f2e2a9 to c9cf398 Compare November 11, 2022 15:43

pquentin added 4 commits November 12, 2022 08:14

Try memray again after 1.4.1 release

4ea7d2d

Avoid len() calls

b24d1da

Change ValueError to RuntimeError

0dc26a3

Prevent decode_content variations

f894e83

Fix mypy

561fb47

franekmagiera mentioned this pull request Nov 13, 2022

Standardize HTTPResponse.read(X) behavior regardless of compression #2712

Closed

pquentin added 2 commits November 13, 2022 21:20

Revert "Prevent decode_content variations"

a2d3a34

This reverts commit f894e83 and 561fb47.

Fix regression on memory usage without decoding

b518302

pquentin force-pushed the extract-raw-read branch from 079811f to b518302 Compare November 13, 2022 17:47

sethmlarson previously approved these changes Nov 13, 2022

View reviewed changes

pquentin added 2 commits November 13, 2022 23:08

Merge branch 'main' into extract-raw-read

6fa19dc

Fix CI by testing on 3.11 final instead of -dev

b869213

The deadsnakes PPA no longer supports 3.11-dev it seems.

pquentin dismissed sethmlarson’s stale review via b869213 November 14, 2022 07:37

sethmlarson approved these changes Nov 14, 2022

View reviewed changes

sethmlarson merged commit c35033f into urllib3:main Nov 14, 2022

This was referenced Nov 14, 2022

add from __future__ import annotations with isort #2779

Closed

Run tests in verbose mode #2810

Merged

sethmlarson mentioned this pull request Jan 2, 2023

Calling .read() on a request returns an empty string #715

Closed

pquentin mentioned this pull request Apr 28, 2023

Fix HTTPResponse.read(0) as first read call #2998

Merged

pquentin mentioned this pull request Jan 2, 2024

void #3250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize HTTPResponse.read(X) behavior regardless of compression #2798

Standardize HTTPResponse.read(X) behavior regardless of compression #2798

pquentin commented Nov 11, 2022 •

edited

Loading

sethmlarson left a comment •

edited

Loading

sethmlarson Nov 11, 2022

pquentin Nov 12, 2022

sethmlarson Nov 11, 2022

pquentin Nov 12, 2022

sethmlarson Nov 11, 2022

pquentin Nov 12, 2022

sethmlarson Nov 11, 2022

pquentin Nov 12, 2022

pquentin commented Nov 12, 2022

sethmlarson commented Nov 12, 2022

sethmlarson left a comment


		data = self._decode(data, decode_content, flush_decoder)
		if not data and len(self._decoded_buffer) == 0:

Standardize HTTPResponse.read(X) behavior regardless of compression #2798

Standardize HTTPResponse.read(X) behavior regardless of compression #2798

Conversation

pquentin commented Nov 11, 2022 • edited Loading

sethmlarson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pquentin commented Nov 12, 2022

sethmlarson commented Nov 12, 2022

sethmlarson left a comment

Choose a reason for hiding this comment

pquentin commented Nov 11, 2022 •

edited

Loading

sethmlarson left a comment •

edited

Loading