Fix: retain stream read error on subsequent response.content access#7415
Fix: retain stream read error on subsequent response.content access#7415hellozzm wants to merge 1 commit into
Conversation
When accessing response.content raises an exception during stream reading, subsequent accesses would silently return an empty bytestring instead of re-raising the original error. This made debugging difficult, especially in debuggers where properties may be accessed multiple times. The fix stores the exception and re-raises it on subsequent accesses. Fixes psf#4965
golikovichev
left a comment
There was a problem hiding this comment.
Nice catch on #4965. The silent b"" on the second access is a real foot-gun, especially in a debugger that touches the property more than once.
The fix works. A few suggestions:
Initialising the error: _content_error is set dynamically in the property, whereas Response initialises its state in init (_content = False, _content_consumed = False). It would be more in keeping with the class to add self._content_error = None in init and check if self._content_error is not None, rather than relying on hasattr. That also gives the attribute a place in pickling. Right now it is not in attrs, so it would not round-trip through getstate/setstate, which is probably fine but worth a deliberate choice.
Short-circuit: as written, a second access still re-enters the b"".join(self.iter_content(...)) path before reaching the re-raise, because _content is still False. On an exhausted stream that just yields b"" again, so it is harmless but unnecessary. Moving the if self._content_error is not None: raise to the top of the property would short-circuit a previously failed response right away.
Test: the snippet in the description is a good reproduction. It would be worth landing it as a regression test under tests/, asserting it raises on both the first and the second access. A behaviour change like this usually needs a test, and it guards the b"" regression from coming back.
Minor: except Exception is broad, but since you re-raise immediately it is reasonable here.
Summary
Fixes #4965
When accessing
response.contentraises an exception during stream reading (e.g.,ConnectionError), subsequent accesses would silently return an empty bytestringb""instead of re-raising the original error.This made debugging difficult, especially in debuggers where properties may be accessed multiple times.
Changes
_content_errorwheniter_contentraises during content readingTesting