Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Handle both exceptions possible while consuming a socket #1940

Closed
wants to merge 3 commits into from

4 participants

@sigmavirus24
Collaborator

This fixes #1939 and part of @zackw's rehaul of redirection.

To better test this, I'd like to add Betamax as a test dependency. Also, as a separate PR I plan to kill the usage of RuntimeError in Response#content. That's awful and the community consensus is that nothing should ever raise that.

@sigmavirus24
Collaborator

And yes, I know I'm using a pattern that I absolutely abhor. That aside, there's no other way to catch the decode error.

I wonder, however, if there's a better way to handle this. I'll sleep on it. :)

requests/sessions.py
@@ -90,7 +90,10 @@ def resolve_redirects(self, resp, req, stream=False, timeout=None,
while resp.is_redirect:
prepared_request = req.copy()
- resp.content # Consume socket so it can be released
+ try:
+ resp.content # Consume socket so it can be released
+ except (ContentDecodingError, RuntimeError):
+ pass # It seems to have already been consumed.

In the ContentDecodingError case, how do we know that the entire content has actually been read?

@sigmavirus24 Collaborator

Good question @mechanical-snail. I see your point that this could break mid-stream or even at the start. How does cd05910 look to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Lukasa Lukasa was assigned by sigmavirus24
@schlamar

The actual point of raising the exception is here: https://github.com/kennethreitz/requests/blob/master/requests/adapters.py#L391

(you need to cover both of them)

requests/sessions.py
@@ -90,7 +90,10 @@ def resolve_redirects(self, resp, req, stream=False, timeout=None,
while resp.is_redirect:
prepared_request = req.copy()
- resp.content # Consume socket so it can be released
+ try:
+ resp.content # Consume socket so it can be released
+ except (ContentDecodingError, RuntimeError):
+ resp.raw.read() # Ensure that the socket is consumed
@schlamar
schlamar added a note

You need to use resp.raw.read(decode_content=False). raw.read does decoding per default.

@sigmavirus24 Collaborator

That's gross but I can understand why that would be the default.

@Lukasa Collaborator
Lukasa added a note

It's the default in hyper too. I'm not happy about it.

@sigmavirus24 Collaborator

Addressed in 3cd198c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sigmavirus24
Collaborator

The actual point of raising the exception is here: https://github.com/kennethreitz/requests/blob/master/requests/adapters.py#L391

@schlamar we cannot catch that in Session#resolve_redirects. To catch it in the adapter would be a serious change in behaviour. I wonder why we consume all of the content in the adapter though. If there's a decoding error, we don't even attempt to return a Response to the user. That may be fodder for an entirely different issue though. If @Lukasa is satisfied with this as an immediate solution to what was reported, I am too. But I want to investigate that other piece in the adapter as well.

@schlamar

satisfied with this as an immediate solution to what was reported

This is no solution to #1939 because it is already failing in Adapter.send (before resolving redirects). See the traceback in this issue. The analysis of @mechanical-snail is not quite correct here.

It would work if we move the if not stream: r.content at the end of Session.send (which is IMO the correct place, the stream argument should only come in play after the redirects are resolved). But that means that the stream parameter to Adapter.send is obsolete, so this would be a API breaking change.

@sigmavirus24
Collaborator

Reviewing the issue, you're once again correct @schlamar. That aside @mechanical-snail is not wrong that this could in fact also be a problem. Their issue won't be fixed by this but it may fix the case where streaming is used.

@sigmavirus24 sigmavirus24 deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 2 deletions.
  1. +6 −2 requests/sessions.py
View
8 requests/sessions.py
@@ -18,7 +18,7 @@
from .models import Request, PreparedRequest, DEFAULT_REDIRECT_LIMIT
from .hooks import default_hooks, dispatch_hook
from .utils import to_key_val_list, default_headers, to_native_string
-from .exceptions import TooManyRedirects, InvalidSchema
+from .exceptions import TooManyRedirects, InvalidSchema, ContentDecodingError
from .structures import CaseInsensitiveDict
from .adapters import HTTPAdapter
@@ -90,7 +90,11 @@ def resolve_redirects(self, resp, req, stream=False, timeout=None,
while resp.is_redirect:
prepared_request = req.copy()
- resp.content # Consume socket so it can be released
+ try:
+ resp.content # Consume socket so it can be released
+ except (ContentDecodingError, RuntimeError):
+ # Ensure that the socket is consumed
+ resp.raw.read(decode_content=False)
if i >= self.max_redirects:
raise TooManyRedirects('Exceeded %s redirects.' % self.max_redirects)
Something went wrong with that request. Please try again.