Response.content retains and re-raises errors from a broken stream#7585
Open
agu2347 wants to merge 1 commit into
Open
Response.content retains and re-raises errors from a broken stream#7585agu2347 wants to merge 1 commit into
agu2347 wants to merge 1 commit into
Conversation
Previously, if reading response.content raised an exception (e.g. ChunkedEncodingError from an incomplete chunked response), the exception propagated on the first access, but _content stayed at its initial sentinel value (False) and _content_consumed stayed False. A second access to .content would then attempt to re-read the underlying stream. Since the connection/stream is often already dead after the first failure, this second attempt silently succeeded with no data, so .content returned b'' instead of raising -- masking the original error entirely. Add a _content_error attribute that stores the exception raised while reading content. If set, .content re-raises it on every subsequent access instead of attempting (and silently succeeding at) reading an already-broken stream again. Added a regression test using a stateful fake raw object that faithfully reproduces the real urllib3 behavior being worked around: raises once on the first stream() call, then yields no further data (no error) on a second call, simulating a dead connection. Confirmed the test fails without the fix (second access returns empty bytes instead of raising) and passes with it. Ran the existing non-network-dependent test suite; results are identical to a clean checkout of main aside from the one new passing test (pre-existing httpbin fixture and network-dependent failures/errors in this sandbox are present on a clean main checkout too, unrelated to this change). Fixes psf#4965
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4965.
Previously, if reading
response.contentraised an exception (e.g.ChunkedEncodingErrorfrom an incomplete chunked response), the exception propagated on the first access, but_contentstayed at its initial sentinel value (False) and_content_consumedstayedFalse. A second access to.contentwould then attempt to re-read the underlying stream. Since the connection is often already dead after the first failure, this second attempt silently "succeeded" with no data, so.contentreturnedb''instead of raising -- masking the original error entirely, as described in the issue.Fix: add a
_content_errorattribute that stores the exception raised while reading content. If set,.contentre-raises it on every subsequent access instead of attempting (and silently succeeding at) reading an already-broken stream again.Verification: added a regression test using a stateful fake
rawobject that faithfully reproduces the real urllib3 behavior being worked around -- raises once on the first.stream()call, then yields no further data (no error) on a second call, simulating a dead connection. I confirmed the test fails without the fix (second access returnsb''instead of raising) and passes with it.Ran the existing non-network-dependent test suite; results are identical to a clean checkout of
mainaside from the one new passing test (pre-existinghttpbinfixture and network-dependent failures/errors in this sandbox are present on a cleanmaincheckout too, unrelated to this change).