Set response attribute length_remaining in BaseHTTPResponse

[programmer-ke]

Closes #3315

Since both HTTPResponse and HTTP2Response use the length_remaining, make it an attribute of the base class.

[programmer-ke]

Changelog test is failing. I don't think a changelog is needed for this since it is invisible to users?

[pquentin]

Thanks for tackling this!

length_remaining needs to be passed in the __init__ method without any default value as is done for status or version. Then in HTTP2 response we can call super().__init__(status=status, ..., length_remaining=0). Does that make sense?

[programmer-ke]

Thanks for tackling this!

length_remaining needs to be passed in the __init__ method without any default value as is done for status or version. Then in HTTP2 response we can call super().__init__(status=status, ..., length_remaining=0). Does that make sense?

That does make sense. Now, in the case of HTTPResponse, length_remaining is initialized by the _init_length. We'd call this before calling super().__init__ passing in the calculated length. However _init_length in turn relies on a few attributes set in the super's __init__.

urllib3/src/urllib3/response.py

Line 614 in 8c8e26d

self.length_remaining = self._init_length(request_method)

How can we handle of this? The easy way would be to simply call super with an initial length_remaining of 0 then updating it with the calculated value after super().__init__.

[programmer-ke]

length_remaining needs to be passed in the __init__ method without any default value as is done for status or version. Then in HTTP2 response we can call super().__init__(status=status, ..., length_remaining=0). Does that make sense?

I've now made it a parameter of BaseHTTPRequest's __init__, what do you think?

Also I've run the CI twice the tests seem to be failing on random network issues.

[pquentin]

Thanks for the proof of concept! I'm not sure what to think. Setting to 0 and then to the real value seems pretty bad, but passing all required values to init_length seems pretty bad too. I'm sorry that I failed to see that, and for now this changed looks a like net negative to me. :/

I'll wait to see if another reviewer thinks differently.

[illia-v]

What do you think about adding an annotation length_remaining: int | None without any default value to BaseHTTPResponse?

[programmer-ke]

What do you think about adding an annotation length_remaining: int | None without any default value to BaseHTTPResponse?

Possible. So do we still need to pass its value as an argument to __init__? If so we'll always have to provide some value there i.e. super().__init__(..., length_remaining=0) , or otherwise specify a default value for the parameter.

[sethmlarson]

@programmer-ke I agree with @illia-v's approach, let's type annotate BaseHTTPResponse and keep the default value of 0 only on the new HTTP2Response.

[programmer-ke]

Made an update, now setting the length_remaining in the base class without a default value.

Also, there's some flakiness in the tests I think

[pquentin]

Thanks! LGTM.