Raibek/requests-github-issues huggingface dataset:
docs don't show correct max_redirects

In the docs, `max_redirects = None` (http://docs.python-requests.org/en/master/api/?highlight=max_redirects#requests.Session.max_redirects)

however it actually is set to `DEFAULT_REDIRECT_LIMIT` from `.models`...

https://github.com/kennethreitz/requests/blob/cd4e6b9aef4b5d3224a1aae1b3a2cef1b09710a4/requests/sessions.py#L330-L332

... which is defined as `30`

https://github.com/kennethreitz/requests/blob/cd4e6b9aef4b5d3224a1aae1b3a2cef1b09710a4/requests/models.py#L47


Hrm, this seems to be a problem with the RTD docs build. @sigmavirus24, any ideas?


So, sphinx is the source of this bug and I suspect it might be fixable but with something that I'm not entirely certain we want to do.

The following triggers the "bug" (a.k.a., limitation in sphinx-doc):

``` py
class Session(...):
    def __init__(self):
        self.max_redirects = 30
```

And this "fixes" it:

``` py
class Session(...):
    max_redirects = 30
    def __init__(self):
        pass
```

We update our attributes to do this for documentation purposes, but this _can_ have negative effects if we over-do it. Thoughts?


I'd rather not do it, frankly: I think it'd end up looking gross. =(


The class (or possibly method) do string can declare it as an instance variable, and then note what package it's imported from.  It's not a perfect solution in that one needs to look at source for the real value, but it stops the wrong info from appearing in the API docs.   (Fwiw, I naively trusted the docs and hit the limit... Making me go crazy for 15 minutes reinstalling requests and trying to figure out why No Limit was triggering an exception)

> On May 31, 2016, at 8:18 PM, Cory Benfield notifications@github.com wrote:
> 
> I'd rather not do it, frankly: I think it'd end up looking gross. =(
> 
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.


@Lukasa to be clear we'd still be able to use the constant we import from `requests.models` that also appropriately helps sphinx determine what is happening.

I am not particularly opinionated about this I guess.


Can we just change the doc string to say "Defaults to `30`"?


@Lukasa that works for me.


@Lukasa that was going to be my suggestion. Utilizing class-instance variables just for the doc build is a huge red flag for me. 


It's important to keep in mind that a _lot_ of people read through the Requests codebase is a way to learn how to write and organize Python code well. 


> Utilizing class-instance variables just for the doc build is a huge red flag for me. 

So class variable definitions have a lot of other benefits. One very common one is if someone uses mock to create an autospec'd Session. The mock will reflect the attributes that users expect to be there _only_ if the attributes were defined at the class level. (In other words, we make people's lives easier when they mock out the Session as a collaborator object in their testing.)
Make RequestEncodingMixin._encode_params() public

I need something that does exactly what `RequestEncodingMixin._encode_params()` does. This works in both python2 and 3:

``` py
>>> RequestEncodingMixin._encode_params({u'a': b'b'})
'a=b'
```

Can it be made part of the public API? (Or is it already?)

I am aware that I can just use it as is. But that seems "wrong".


I don't have any strong feelings on it. I actually am uncertain why we're using a Mixin there when those are static methods and don't need to be declared on a class.

If we're going to make them public, I'd rather move them to a well documented submodule that does encoding of data and such for requests. That way people can simply do:

``` py
from requests import encodingutils

encodingutils.encode_params(...)
```


Do we really need to expose them? This is a 16-line function that mostly just translates different inputs to something useful to pass to `urlencode`. Do we need to really expose this, or is it safer just for those who want it to copy the code out?


> Do we really need to expose them?

I don't know. I do have questions about using a Mixin here but that's tangential to this issue. I'm not sure we want to make these publicly available though. I lean towards not doing it because it's not what requests is for, but I'm also not sure if not opening it up means people won't use it anyway.


Don't change something just because you can.
Session cookies handled incorrectly on redirect

Hi everyone,

the session cookies are handled incorrectly when being redirected from an https URL to an http resource.The initial cookies have been acquired via an https request to <domain>. Therefore, the secure attribute of the respective cookie is set to True. In a later request to the same domain via https, I am 307 redirected to the very same URL except via http. When requests tries to resolve the redirect it will not copy the cookie over because the `return_ok_secure` check in the `cookielib.py` will obviously fail (mismatch between request.type == 'http' and checked type 'https'). Is that a wrong implementation by the service provider or python-requests? Below a pseudo example:

```
import request
print requests.__version__
>>> '2.10.0'
s = requests.Session()
s.post('https://domain.com/login', data={'login': login, 'pw': pw})
print s.cookies
>>> <RequestsCookieJar[<Cookie token=... for domain.com/>]>
res = s.get('https://domain/com/resource') # redirects internally with 307
```

Unfortunately, I can not provide a minimal working example since the script would include sensitive information.


The `secure` attribute of a cookie should only be set to `True` if the service provider sets it manually. If they _do_ set it manually, that is an instruction that the cookie must not be sent over an insecure (e.g. `http`) channel. Requests is correctly refusing to send that cookie: it should only be sent when TLS is used.
Use xfail marker for a test expected to fail without Internet connection

This is only a minor improvement on the great work of https://github.com/kennethreitz/requests/pull/2859 that permits to run tests on hosts without Internet connection without failures.


Not sure why no-one's gotten back to you on this yet — my apologies. 

This looks like a great improvement. Thank you. 


@eriol I'm so sorry. I think I lost track of this while I was travelling to PyCon. I expect the same happened to @Lukasa 


Sorry for my late reply... No need to apologize! I was aware of the PyCon during those days and I was sure this would not be lost... so just thanks for merging it! :)
Convert readthedocs link for their .org -> .io migration for hosted projects

As per their email ‘Changes to project subdomains’:

> Starting today, Read the Docs will start hosting projects from subdomains on the domain readthedocs.io, instead of on readthedocs.org. This change addresses some security concerns around site cookies while hosting user generated data on the same domain as our dashboard.

Test Plan: Manually visited all the links I’ve modified.


Just realized these are all in `urllib3`, replaced with shazow/urllib3#882
Global verify options

I have noticed that it is possible to specify `verify=False` in order to avoid `CERTIFICATE_VERIFY_FAILED`. Is it possible to globally set this option?


It is not, no.

Setting `verify=False` is an extremely insecure thing to do, and should be avoided if at all possible. Having a hook that enables you to _globally_ disable TLS certificate verification is extremely insecure, and is also where we draw the line.


Further, we request that all questions be asked on [StackOverflow](stackoverflow.com), not on the defect tracker.
Session.request() overrides socket.setdefaulttimeout() 

Hello,

I am using requests 2.10.0 and Python 2.7.11.

By setting `timeout=None` by default in the `session.Session().request()` method's default parameters, the value `None` is propagated all the way to the low level `socket` module, overriding whatever value was set using `socket.setdefaulttimeout(_timeout_)`.

If the user doesn't assign a timeout value, the expected result is that `socket.getdefaulttimeout()` is used. To get this behavior, the value of `packages.urllib3.util.timeout._default` should be used by default instead of None, since None has a special meaning. This can be done by importing `packages.urllib3.util.Timeout` as `TimeoutSauce` (as it is done in the `adapter` module) and assigning `timeout=TimeoutSauce.DEFAULT_TIMEOUT`.

Here is a unit test suite that shows that using `socket` directly respects `socket.getdefaulttimeout()` and another test using `requests` that blocks indefinitely unless the above modification is applied.

```
# usage: python -m unittest discover
import socket
import unittest

import requests

class SocketTimeoutTestCase(unittest.TestCase):

     def setUp(self):
         self.original_timeout = socket.getdefaulttimeout()
         socket.setdefaulttimeout(0.1)
         self.host = '127.0.0.1'
         self.port = 50000
         self.server = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
         self.server.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
         self.server.bind((self.host, self.port))
         self.server.listen(1)

     def tearDown(self):
         socket.setdefaulttimeout(self.original_timeout)
         self.server.close()

     def test_default_timeout_with_socket(self):
         client = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
         with self.assertRaises(socket.timeout):
             client.connect((self.host, self.port))
             conn, addr = self.server.accept()
             client.recv(1024)
         conn.close()

     def test_default_timeout_with_requests(self):
         with self.assertRaises(requests.Timeout):
             r = requests.get('http://{}:{}/'.format(self.host, self.port))
```

May I submit a pull request? I need to read contributing guidelines to prepare it correctly as this would be my first pull request. As far as I know (and I am not familiar with the code base), only the `session` module requires modification, but you may know better.


This is related to 
- https://github.com/shazow/urllib3/issues/655

Which has been fixed for 3.0.0 because it's otherwise not backwards compatible.
did it update doc?

I  translate Doc into Doc-KR.

in todo.rst

> As of v1.0.0, Requests has now entered a feature freeze.

did this translate or delete?


That sentence is true, but it's still fine to continue with translations. =)


@lukasa Thanks :) i think that sentence is unnecessary in v2.10.0, so seek your advice.


It's still necessary: we're still in a nominal feature freeze. =)


@Lukasa make sense!! thanks =)
requests does not provide a way to send full chain-of-trust when server makes a CertificateRequest

I am on python2.7 installed on CentOS 6. I have OpenSSL 1.0.1e. I have requests version 2.9.1. The HTTP server is performing SSL client authentication. The client's chain has a verification depth of 2, which means there's a single intermediate CA certificate in the client chain. I am not sure if this is a bug or a feature request.

Based on the public api, I sent a request in like so. note i have explicitly chosen not to verify the server's chain of trust (i know that's not secure but this isn't being used in production). The cert.pem contains the client's PEM-encoded certificate, the intermediate CA cert, and the root cert. The final entity in the cert.pem is the client's private key (which is NOT encrypted).

resp = requests.get(url,verify=False,cert='cert.pem')

The following exception is thrown.

requests.exceptions.SSLError: [Errno bad handshake](-1,)

Looking at the server's ssl logs, I can see that the reason the server terminated the SSL connection was caused by the server's inability to construct the client's chain-of-trust. The server could not do this because the request's library only sent its client cert in it. I used openssl s_server to verify this as well.

I could not find anything in the official documentation that would allow me to specify a set of certs to be included in the client's chain-of-trust. 

Looking at the pyopenssl wrapper used by requests, I can see the Context.use_certificate method, which sets the leaf cert. I am suspecting that only sets a single certificate. I suspect that for this to work, the requests external api would need to provide an interface for passing in a list of PEM-encoded certs, which is then set via the Context.add_extra_chain_cert method.


Hi, thanks for this report!

The PyOpenSSL wrapper here is actually part of [urllib3](https://github.com/shazow/urllib3), and requests uses urllib3 exactly as provided in that project. Do you mind opening this issue over there instead? (It'll seem weird because it'll still be me responding to you there, but trust me, this is organisationally cleaner for us.)


Sure!! Not a problem.
uncaught possible exception in urllib3/connectionpool.py

Hi,
I'm seeing the following exception

```
File "/opt/virtualenvs/myapp/lib/python2.7/site-packages/requests/packages/urllib3/connectionpool.py", line 314, in _raise_timeout
    if 'timed out' in str(err) or 'did not complete (read)' in str(err):  # Python 2.6
TypeError: __str__ returned non-string (type SysCallError)
```

I'm using version 2.7.0 of requests. The url I try to access is a https one.
I don't really know how to debug this... 


Please update your version of requests. This was fixed several releases ago.


Thanks. Updated now. Will reopen if needed.
unhandled exception on timeout

``` python
Traceback (most recent call last):
  File "requests/packages/urllib3/connectionpool.py", line 623, in urlopen
    _stacktrace=sys.exc_info()[2])
  File "requests/packages/urllib3/util/retry.py", line 285, in increment
    _observed_errors=_observed_errors)
  File "requests/packages/urllib3/util/retry.py", line 151, in new
    raise_on_status=self.raise_on_status,
AttributeError: 'Retry' object has no attribute 'raise_on_status'
```

You can reproduce this by changing line 140 of `requests/packages/urllib3/connection.py` to:

```
140         try:
141             raise SocketTimeout('')
142             conn = connection.create_connection(
143                 (self.host, self.port), self.timeout, **extra_kw)
144
145         except SocketTimeout as e:
```

This forces a `SocketTimeout` and causes the unhandled exception when calling `Retry.new`.


Hey @alanhamlett, thanks for reporting this!

This will be fixed when we release the next minor version of requests, which will bring in an updated version of urllib3.
Request's documentation is self-contradictory about whether connections are pooled automatically

The introduction to requests claims,

> Keep-alive and HTTP connection pooling are 100% automatic

The page on sessions contradicts this, and heavily implies that I need to create a `Session` object to get pooling:

> if you're making several requests to the same host, the underlying TCP connection will be reused, which can result in a significant performance increase (see HTTP persistent connection).

If I need to create a Session in order to achieve connection pooling… that doesn't seem "100% automatic" if I must manually ask for it.

Use of the library seems to indicate that `Session` is the way to go, because I'm getting spammed by "Starting new HTTPS connection" logs.


Thanks for this!

I don't know that I'd call the documentation contradictory. At least when I see "100% automatic" I see a phrase that means "no user intervention is required", which is true. However, you do need an object to put state on to.

Regardless, what wording change would you recommend to clear this up?


Ever porcelain-level api call (e.g. `requests.get`) has a session baked into it, which will be used across all requests within that function call.

As documented, if you intend to make multiple calls, you must create a session.

Documentation is correct.
Document the cert pooling bug

Hi there.  I just ran into https://github.com/kennethreitz/requests/issues/2863 and spent a lot of time trying to figure out what was wrong.  It would be really nice for future people who might encounter this bug to be informed up front, so here is a note to add to the User Guide and the API documentation.

I understand that people are working on fixing this bug, but it's not fixed yet and it's been open for about 6 months.  I think it's valuable to make this quick change to the docs in the meantime, and then when the bug is fixed we can remove these warnings.

Thanks!


Thanks for this suggestion! However, I don't think the proposed documentation change is good: in fact, I think it is dangerously misleading. The bug occurs regardless of how `cert` is set: so long as it has been set and a session is being used it is possible for it to be effectively incorrectly set. 

However, I also think there is no urgency to document this. A fix has been merged into urllib3 (shazow/urllib3#830), and a new urllib3 release will be out in the next few weeks. I doubt this will remain unfixed for more than another few weeks. 


Thanks, I didn't know a fix was imminent.  I'm looking forward to it!

I don't understand what's wrong with the advice to set `cert` immediately on creation and never changing it—only asking for clarification in case it means I don't understand what the bug is.  If you set `cert` before making any requests and never change it, then how can you encounter the bug?


Nothing's wrong with that advice: it's just not what your documentation says. =) Your documentation change says _don't use it at all_, which is not necessarily right.

You're right though: if you set `cert` before using the Session and never want to change or override it, you won't hit the bug.
Not possible to receive server response before all of request body is sent?

I'm not sure if this is actually the case, but from looking at the code it appears to be. To motivate my use case, I am using chunked transfer-encoding mode for the request since I want to support streaming from an underlying service, so the content length is unknown and could be very long. It appears that if the server sends a response prior to the request body being completely sent, we wouldn't know about it, which would be quite bad for something like a redirect or an error condition that was detected very early in the stream. In particular, in a redirect case we certainly don't want to re-send the entire stream.

Is there a good way around this?


@zachmullen short of rewriting our HTTP stack to not use httplib, no. And none of us presently have the time to do that. :-(


Well, we could work around it if we adjusted `send` to always check whether the socket is readable first, but it's a dirty hack and I don't think it's worth it: it'll just be too brittle I suspect. That said, if someone wants to prototype a change to do that in urllib3 it'd be an interested experiment.

Regardless, this would first be a urllib3 enhancement so I'm closing this.


Ok, thanks


(Closing for @Lukasa since he said he was going to and didn't. ;))
Issue #2645 - Change behavior of Response.ok

As discussed on issue #2645, I added a success attribute to
requests.codes. Now, Response.ok checks if the response's status code
is listed on requests.codes.success.


I get it, it's just that I thought the agreed solution was to add the success attribute to requests.codes. I'll update the commit right now.


This LGTM. We can't merge until we're ready to do 3.0.0, though.


An additional check should be added for entire 2xx range after the registered codes are checked (or perhaps the '-1 index +1 :' slice). Both for future-proofing and a server might be using an esoteric number honoring the numbering scheme. 


Also, I've shared some thoughts about this on https://github.com/kennethreitz/requests/issues/2645.

Basically, no behavioral modifications to `ok`. A new attribute can be added for the 200-only functionality, if this is still desired. 

I will need to think for a bit about the naming. feel free to use `is_successful` as a placeholder for now.

---

Maintainers: **do not merge** without my approval.


P.S. everybody gets cake :sparkles: :cake: ✨
session proxies not honored during request

Using a session object and setting the proxies dictionary does not affect the proxies used during a request.

ex: Still tries to use system proxy.

```
s = requests.Session()
s.proxies = {'http': None, 'https': None}
s.post(url='http://10.0.1.1', json={'test': 'data'})
```

Using proxies during each request works, but it would be great to set them at the session level. Specifically if you potentially have dozens or ".post()" or ".get()" calls throughout your script.

Any thoughts?


Thanks for this! This is a duplicate of #2018.
redirect with odd (but correct) Location url fails via proxy

Using v2.10.0 (but tested and get same behavior with 2.9.1), if we get a 302 redirect with a header like this (note the missing trailing slash after the domain):

Location: http://captrust.netxinvestor.com?custId=....

AND we're using a proxy (proxy in question is tinyproxy, but tested with other proxies and get same behavior)

AND allow_redirects=True

the proxy will fail with "cannot connect" because the domain in the Location url is improperly parsed.

Firefox and Chrome both properly handle the redirect.  To be honest I haven't checked whether the Location URL does or does not violate the spec, but if the browsers handle it and it appears to be unambiguous I'd vote for `requests` handling it also.  Seems like an easy patch, though I must also confess that I've stepped through the code and can't find where to handle it.

Oddly, if that exact URL is passed to requests as a GET, even via a proxy, it works fine -- so my guess is that it happens somewhere in the SessionRedirectMixin object.

Workaround right now is to request with allow_redirects=False and handle the redirect manually.


Are you sure this is a URL parsing issue? Are you getting an exception or an error from the proxy? If the former, can I see it?

If not, which is what I suspect, I think your proxy may be the one that's struggling. Essentially, the major difference in the two code paths is that requests won't add the `'/'` path to a `Location` header. Now, probably it should, but right now it doesn't. That means that in the case when you pass it in to requests yourself, requests would emit an extra `/` to the proxy. It seems that your _proxy_ is getting confused here, more than requests.


I can't be sure, though I'll give it another test to see if I can watch where it submits to the proxy.  But without question it works to request the Location URL, but it fails with a 500 error if I request a URL that redirects to the Location URL.  Since the proxy is the same both ways, my best guess is that requests is asking something different of the proxy.


Yeah, that's definitely the case, but I think the two URLs are semantically identical.

Regardless, I _also_ think that we have a question to ask ourselves (we being @sigmavirus24 and I), which is: why does `resolve_redirects` not end up calling `prepare_url`?
Fix to check for Plain ip notations in no_proxy settings if not CIDR

For ipv4 addresses no_proxy is not being honored. 

[This line ](https://github.com/kennethreitz/requests/blob/master/requests/utils.py#L539) checks for `cidr notation` but `plain ip` notation is not considered, due to which the request is always routed to proxy server. This results in error ReadTimeoutError

Sample `proxy` configuration

```
no_proxy=192.168.1.1,192.168.1.3,*.example.com
http_proxy=http://192.168.1.100:8080
https_proxy=http://192.168.1.100:8080
```

> When request is made to `192.168.1.1`, the request should not be made to proxy server, without this fix it is being made to proxy server `192.168.1.100`


This seems good to me. Can we have a test to validate that this function works as expected?


@Lukasa added tests, please check and merge.


@Lukasa addressed comments, pls check


Cool, I'm :+1: on this for now. I'd like another :+1: from @sigmavirus24 before we merge, but I think this is good.


Looks good to me. Thanks @kumarvaradarajulu
Add support for in-memory multipart upload

As far as I can understand from the docs, it is possible to upload files using the 'files' keyword param, but it requires a file object. I have a case where I have the data in memory, and it would require me to store to a temp file, which I would like to prevent if possible.
Would it be possible for requests to support in-memory data upload for multipart?


It already does. =) Note the description for the files kwarg in [this bit of documentation](http://docs.python-requests.org/en/master/api/#requests.request).


Also [this section of the narrative documentation](http://docs.python-requests.org/en/master/user/quickstart/#post-a-multipart-encoded-file) mentions it.


But it requires a 'file-like object', doesn't it? What I meant is I have a byte buffer in memory (an instance of the bytes type in Python 3), not a file. For this interface to work I have to write to a temp file and upload this (at least that's what I am doing now).


> But it requires a 'file-like object', doesn't it?

No. Let me quote [the narrative documentation](http://docs.python-requests.org/en/master/user/quickstart/#post-a-multipart-encoded-file) here:

> If you want, you can send strings to be received as files:
> 
> ```
> >>> url = 'http://httpbin.org/post'
> >>> files = {'file': ('report.csv', 'some,data,to,send\nanother,row,to,send\n')}
> 
> >>> r = requests.post(url, files=files)
> >>> r.text
> {
>   ...
>   "files": {
>     "file": "some,data,to,send\\nanother,row,to,send\\n"
>   },
>   ...
> }
> ```


Right, sorry, I was a bit mislead by the 'string' wording in the example. For me, it didn't seem like an instance of bytes could also be passed. I guess the documentation could be improved on this - the 'files' keyword description should probably say it is not really necessary for it to be a fileobj, and the narrative docs should say it can also be a bytes instance, not only a string. (Although it rings a bell that in Python 2.x things were calles 'byte strings', maybe that's where it comes from?)

But thank you anyways, I tested it and it works, and I prefer this way than mess around with temporary files.


> (Although it rings a bell that in Python 2.x things were calles 'byte strings', maybe that's where it comes from?)

Yup. In Python 2, strings were by default byte strings, which is very similar to the Python 3 `bytes` type. This convention has stuck around for those of us who write Python 2/3 compatible code.
Allow graceful interruption of testserver.Server

So that failing tests don't cause the server thread to hang
indefinitely, waiting for connections that will never come.

Rationale for suppressing error/traceback from interrupted
_accept_connection in testserver.Server:
https://gist.github.com/brettdh/b6e741227b2297f19d2118077f14dfa5


ping @Lukasa @sigmavirus24 

I think I addressed the comments from #3185.

Oddly, I cannot make the test suite hang indefinitely when I run it locally, but that's what's happening in jenkins. :confused: Maybe you can spot something I'm missing?


Ok, cool, I'm happy with this. Assuming @sigmavirus24 is as well, he may merge.


I'm going to squash merge because the title of 5cfb691 is not very helpful and I don't want it in our commit history if we go to bisect things.


Thanks @brettdh!
GAE and ChunkedEncodingError, working patch.

Hey everyone,

sorry for my intervention here, "out of the blue". I tried to understand if this issue was already discussed (but could find them) so I come here with the situation and trying to understand the solution.

The problem is the following (from what I understood) : Google App Engine, replace the implementation of urlfetch by their own, which contains some changes that breaks urllib3 and subsequently Requests.

@agfor [made a fix](https://github.com/agfor/requests/commit/da863cc) that seems to work (I didn't had any ChunkedEncodingError since I place the patch on line) and, from my understanding, doesn't break the others users, those that don't use GAE.

I didn't saw his patch as submitted here (I'm wondering why) and I don't know why this hasn't been taken into consideration here. (Please note those are real question, not critics/remarks, my intentions are good :) ).

Hence my post :)


@cnicodeme The requests project does not consider GAE formally supported. Put another way, we don't have any special-case code _in requests_ to use GAE properly. The recommended method of using GoogleAppEngine with requests is to use the [requests-toolbelt `AppEngineAdapter`](http://toolbelt.readthedocs.io/en/latest/adapters.html#appengineadapter).


Ok that explains all my questions, thank you !

(and wow, you're fast!)


My pleasure!
Support ALL_PROXY environment variable

Closes #3183.


ping @Lukasa 


Agh, Python3 (iteritems). Pushing fix with six shortly.

On a related note - has there ever been talk of moving to [tox](http://tox.readthedocs.io/en/latest/) for testing?


Found that this doesn't actually work yet, as evidenced by the new, failing functional test. Will investigate further (and fix the other broken test) tomorrow.

Also, the jenkins setup doesn't seem to have PySocks installed. What's the best way to make sure that's present in the CI environment? Just add it to the requirements.txt?


@brettdh Yeah, I think that would be fine. =)


Alrighty, I think it's good now.


It's rather difficult to review this with the amount of refactoring of the testserver. It's distracting from the actual feature and reviewing that.


@sigmavirus24 The test server refactoring was mainly done as I added a failing functional test for the feature, in order to prevent that test failure from hanging the server (because it was waiting for a connection that wasn't coming).

Now that the test is passing, I'm happy to separate the test server refactoring into a separate PR if that helps.


Ok, I split off all but the minimum changes needed for the new test. I'll make a new PR for the test server refactoring soon.


Cool, @sigmavirus24 do you want to re-review?


Eh I realize you just trimmed that whitespace, but you drew my attention to it ;). Otherwise, this looks okay to me.


@sigmavirus24 Yeah, sorry; I forgot that I had vim configured to always trim trailing whitespace on save. Thought I had reverted those before committing, but clearly I missed one. :-)


@brettdh no need to put trailing whitespace back, just change that line to use iter instead =D


Ok, :+1: from me. @sigmavirus24?


Looks good to me. Thanks @brettdh!
Feature request: support ALL_PROXY environment variable

As of 2.10.0, I can set `HTTP_PROXY=socks5://example.com:port`, and then requests will use that SOCKS proxy. However, other tools (e.g. Docker) currently require the use of `ALL_PROXY` instead, and get confused if you set `HTTP_PROXY` to a URL that begins with `socks5://`. I think Docker should support the use of `HTTP_PROXY`, but requests should also support the use of `ALL_PROXY`, especially with the recent (thank you thank you thank you) addition of SOCKS support.

I thought that this might need to go through urllib3, but it seems like it would be a relatively simple change to [sessions.py](https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L212), to look at the `'all'` key before checking the scheme. But, then I read your contributor's guide and thought it best to post an issue before just doing it. :smile:

Please let me know what you think.


Thanks for the suggestion!

I don't think we need to go through urllib3: your analysis is right, sessions.py can DTRT here. I think this is a reasonable enhancment, so we'd welcome a pull request for this.


Great, thanks for the super-quick answer! I'm happy to pick this up and will start looking into it shortly.
Sending too much personal information in User-Anget. Risky

```
>>> r.request.headers["User-Agent"]
'python-requests/2.x.x CPython/x.x.x Linux/3.xx.xx-generic'
```

I think requests is sending too much info.
The bot name `"python-requests" would be okay.

But my **Python version which may be vulnerable** ?

My Linux **kernel version which may be vulnerable**?

I consider this a big problem. Many people using requests may not be aware of this and inadvertently sending this info.

Please just change the the `User-Agent` to something like
`"python-requests"` or `"Requests"`


@Logmytech We did. Note our [changelog](http://docs.python-requests.org/en/master/community/updates/#id6), which says that we updated in 2.8.0, released 7 months ago. =)
Potential fix for #3066 (Transfer-Encoding and Content-Length headers both being set)


This looks good @davidsoncasey, thanks so much! Do you want to open a parallel pull request with your idea about refactoring `prepare_body`? I'd love to see both side-by-side.


@Lukasa sounds good. I'll work on getting something put together in the next couple of days.


I'm going close this PR out since these changes and discussion have been moved to #3184.
OSError: [Errno 22] in self.sock.sendall(data)

env: Mac OS X El Capitan / python 3.5.1 / requests 2.10.0  

I want to upload file which is about 3GB size.

``` py
    def read_in_chunks(file_object, chunk_size=4096):
        while True:
            data = file_object.read(chunk_size)
            if not data:
                break
            yield data

    with open('3GB.mov', 'br') as f:
        data = b''.join([chunk for chunk in read_in_chunks(f)])

    requests.put(url, headers=headers, data=data)
```

```
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/urllib/request.py", line 1240, in do_open
    h.request(req.get_method(), req.selector, req.data, headers)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/http/client.py", line 1083, in request
    self._send_request(method, url, body, headers)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/http/client.py", line 1128, in _send_request
    self.endheaders(body)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/http/client.py", line 1079, in endheaders
    self._send_output(message_body)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/http/client.py", line 913, in _send_output
    self.send(message_body)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/http/client.py", line 885, in send
    self.sock.sendall(data)
OSError: [Errno 22] Invalid argument
```

Could you give me some advice?


Hi @AstinCHOI,

This is a defect tracker, not a question and answer portion of the requests project. If you're looking for help (which it appears you are) please pose questions on [StackOverflow](https://stackoverflow.com/questions/tagged/python-requests). When posting there, please be prepared to answer questions such as:
- Why are you using a generator and then joining it into a single byte string in memory?
- Why does your traceback not include any code from the requests library?


Hi, @sigmavirus24 
1) http://stackoverflow.com/questions/11662960/ioerror-errno-22-invalid-argument-when-reading-writing-large-bytestring and it's my fault to use like that.
2) Sorry, I couldn't include the traceback with requests due to some logic.. but there is problem when you use data=data which is above 2GB size binary in OS X. If you use OSX, you can test it.

Naver mind. I will post in StackOverflow.

Thanks.


In OS X, alternative way (for some versions): http://stackoverflow.com/questions/2502596/python-http-post-a-large-file-with-streaming and it can post/put with big files.

It was not requests module fault.

Thanks.
Fix TypeError when get json-encoded content of a response

[`self.content`](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L728) could be `None`, so `len(self.content)` may raise `TypeError: object of type 'NoneType' has no len()`


Huh. I feel like maybe we should just change `Response.content` to never be `None`. It's certainly surprising to me that it can be. @kennethreitz @sigmavirus24?


@Lukasa so we set it to `None` in [the descriptor](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L728) in (2) [error](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L739) [cases](https://github.com/kennethreitz/requests/blob/master/requests/models.py#L744). 

I don't think we can just change it to never be `None` before 3.0 but that's my hesitancy to break user-level code speaking.


@sigmavirus24 Yeah, that seems reasonable. In that case, clearly the `json()` code needs to handle it too.

That means I think this patch is fine. @messense are you interested in adding a test to validate that this works as expected?


This came up yesterday in a work context.  The content of a response CAN BE None, and that would be correct, in the case of a 204 response code.
https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

I agree it would be better if response.json() handled that gracefully rather than dying of a JSON parsing error.


I just realized that fix this will make it raise `ValueError: No JSON object could be decoded` instead because of `json.loads('')`


Hurrah, this looks great to me! I'd argue the previous behaviour was a bug so we have no API compatibility concerns to worry about here, but I want @sigmavirus24 to ACK that before merging.


This looks fine to me :)
Encoding JSON requests to bytes for urllib3 to handle

Implements #3177.


@haikuginger Thanks for this! I've left some notes inline.


Bump! 🙂


Sorry @haikuginger! GitHub doesn't ping me when you push new changes. :(


I'm :+1: on this if @Lukasa is =D


Let's do it! Thanks @haikuginger, you're doing awesome work! :sparkles: :cake: :sparkles:
add ability to control the headers that will be deleted in case of redirect

In some cases the redirect is done to a different host.
when the {"Host": "www.example.com"} is set in the headers and the request is redirected to another host, let's say www.another-example.com, the next request will be sent to a new URLwith a wrong Host header.


This is up to @kennethreitz, but I'm disinclined to want to add this: I think I'd rather factor out the logic in `SessionRedirectMixin` as we've done with a few other bits to allow easier subclassing, rather than add further keyword arguments.


I agree with @Lukasa. I don't think we need this as an extra parameter to this method. I also don't think we need to keep track of which headers we're sanitizing for a user. I also agree that we might be able to better serve you by refactoring things so you can override these with subclassing.


👍 


@sigmavirus24, @Lukasa thanks for the comments.
Inheriting `SessionRedirectMixin` was my first idea but then I've seen so much logic in the `resolve_redirects` function so it became a sort of code duplication.
By saying this, I agree with you guys, factoring out the logic might come very handy.
So how should we proceed?


@adaronen If you'd like to supply a PR that factors out this logic to a method (`resolve_*`) then we'll happily review that. =)


Cool, I'll do my best.


Closing due to inactivity. Please feel free to reopen if you have the time to make more forward progress.
Gzipping requests

Good day!

Does `requests` functionality of encoding POST requests with `gzip`?

If no, will you accept a pull request? Some notes, maybe?

Thanks!


@kuraga Requests does not currently support doing this automatically. =) However, doing so is fairly simple. I don't think we'd accept a pull request adding this functionality (it's not likely to be generally useful), but we'd certainly be happy to add an example to our documentation.


Ok, thanks!

P.S.

> it's not likely to be generally useful

Disagree =) But yes, one more dependency...
how to use socks proxy ?

The new feature socks proxy is supported，but how to use it ?

 no example In [http://www.python-requests.org/en/master/user/advanced/#proxies](http://www.python-requests.org/en/master/user/advanced/#proxies) .


Yup, that's a good spot.

To do it, you need to `pip install requests[socks]`, and then you can set the proxy as normal: `proxies={'http': 'socks5://host:port', 'https': 'socks5://host:port'}`.


Thanks and  I got it @Lukasa 


The pull request #3173 should add the needed section. Thanks for the report @iliul! :sparkles:
Possible NameError if unexpected encryption algorithm encountered.

`HTTPDigestAuth.build_digest_header` may fail with a NameError if encryption algorithm is not one of
 'MD5', 'MD5-SESS' or 'SHA.
https://github.com/kennethreitz/requests/blob/master/requests/auth.py#L117


Yup, that looks wrong. Looks like that name needs to be initialised to `None`. Want to provide a patch?


I thought I'd leave that to those of you who know the code base, in case there was something more subtle going on. I'm happy to make a PR if you want.


PRs always appreciated!
requests.cookies hardcodes a root path attribute

While trying to figure out how to set specific [path attributes](https://en.wikipedia.org/wiki/HTTP_cookie#Domain_and_Path) in cookies when using requests, I noticed that `RequestsCookieJar` [hardcodes the root path](https://github.com/kennethreitz/requests/blob/bbeb0001cdc657ac8c7fef98e154229bc392db0e/requests/cookies.py#L400) when cookies are created and doesn't seem to allow this default to be overridden.

Is there a reason for this? If not, would the maintainers be open to a PR modifying `create_cookie` to heed a `path` specified in `kwargs`?


@evnm That reading of the code is not right. =)

You'll notice [here](https://github.com/kennethreitz/requests/blob/bbeb0001cdc657ac8c7fef98e154229bc392db0e/requests/cookies.py#L414) that we call `update` with the kwargs. That will override the path if you provide it. Try it: `create_cookie('key', 'value', path='/some/path')` should work just fine.


Ah, cool. An object lesson on careful code-reading. Thanks for taking the time to point that out!


No problem, thanks for checking with us! :sparkles:
Extensions Guide

We currently have a list of _recommended_ (e.g. softly endorsed) extensions in the documentation (this needs some love). 

In addition, we should have a document that lists all community extensions, grouped by category.

---

Prompted by https://github.com/kennethreitz/requests/issues/3131


Feel free to paste extensions you're aware of here.


PyPI catalogs these for us see:
- [searching for requests on PyPI](https://pypi.python.org/pypi?:action=search&term=requests&submit=search)
- [searching for requests on Warehouse](https://pypi.io/search/?q=requests)

Judging by the number of these packages, I'm not sure we want to keep a list of _every_ community extension. That seems... extreme.


Yeah, no API clients (maybe one or two exceptions; doubtful). They have to look at least somewhat generally useful.


I forsee this being a fairly large list, intended to be for discovering new toys, or looking for something specific via ctrl-f.


you can start with some well used n promising ones first
requests cannot fetch this website (only in Python 3)

`requests.get("https://www.northwarks.gov.uk/")` hangs.
`urllib.request.urlopen("https://www.northwarks.gov.uk/").read()` works fine.

I am using Python 3.4 and requests 2.9.1.
If I use Python 2.7.10 and requests 2.9.1 then it works fine.


So, first things first. If we set `allow_redirects=False` and `stream=True`, then the first request works. With anything else set, it hangs. The headers the server sends sends seem to be the problem here. Here's the header block curl is seeing:

```
HTTP/1.1 302 Moved Temporarily
Content-Type: text/html; charset=UTF-8
Location: https://www.northwarks.gov.uk/site/
Server: Microsoft-IIS/8.5
X-Powered-By: ASP.NET
X-FRAME-OPTIONS : SAMEORIGIN
Date: Tue, 26 Apr 2016 17:38:24 GMT
Content-Length: 158
```

Given that header block, Python 3's `http.client` module (which is responsible for header parsing), only parses up to `X-Powered-By`. The reason is that the header after (`X-FRAME_OPTIONS`, weirdly all-capped) is not formatted correctly. RFC 7230 forbids a space between the header name and the colon. This breaks the header parser, which therefore doesn't read the Content-Length header field. That means that requests expects that the connection will be framed by connection close, so it barfs.

The overly-strict header parsing you're seeing here is the fault of the Python standard library. You can raise the issue with them: however, they handled a similar one in [bug 25539](https://bugs.python.org/issue25539) and didn't end up moving forward with a fix in Python.

More realistically, you should contact the server operator and tell them that their server is emitting invalid HTTP. This appears to be a local government server, so it should be possible to get in touch with the operators.

Unfortunately, requests can't do much about this: the standard library is just getting this wrong.


Thanks for the excellent analysis. I will look at the `http.client` issue and I agree that it looks like this is not something requests can fix itself.


Thanks for the report @jribbens! :sparkles:


`X-FRAME-OPTIONS%20` :trollface:
utils: let select_proxy not raise an exception when url has no hostname

Fixes #3131.


While we're here, we should probably also handle the possibility that there's no scheme in the URL as well.


scheme defaults to `''`, though. Well, more or less:

> The scheme argument gives the default addressing scheme, to be used only if the URL does not specify one. It should be the same type (text or bytes) as urlstring, except that the default value '' is always allowed, and is automatically converted to b'' if appropriate.


Urgh, `urlparse` is such an inconsistent function. Good spot.


Thanks! :sparkles: :cake: :sparkles: