Use HTTPHeaderDict in request #679

sigmavirus24 · 2015-07-18T04:48:23Z

Supersedes #633

Fix dict(HTTPHeaderDict(other_dict)) and deal with changes in how we implement the HTTPHeaderDict as a MutableMapping subclass instead of as a dict subclass.

sigmavirus24 · 2015-07-18T04:48:55Z

This might not be perfect, but it at least passes the test_collections tests. And it's late here =P

sigmavirus24 · 2015-07-18T04:50:37Z

Tests are failing because there isn't that tests the get method directly of the HTTPHeaderDict.

sigmavirus24 · 2015-07-18T04:53:18Z

urllib3/_collections.py

            yield val[0], ', '.join(val[1:])

    def items(self):
        return list(self.iteritems())

+    viewitems = items


Forgot to delete this. We don't need it.

Lukasa · 2015-07-18T08:38:22Z

Broadly LGTM.

shazow · 2015-07-18T10:45:24Z

Mmmm yea this is a smaller change than I thought we'd need. If you can make the remaining test(s) pass, I'm all for it. :)

shazow · 2015-07-18T10:46:22Z

Btw, 🍮.

Now when we send data with the HTTPHeaderDict we will use the original casing, e.g., if we create an instance with {'COOkie': 'value'} Then dict(HTTPHeaderDict({'COOkie': 'value'})) == {'COOkie': 'value'} So we should expect that the returned headers preserve the capitalization that was provided to the HeaderDict.

sigmavirus24 · 2015-07-18T14:05:48Z

So the tests should pass now. The thing that I'm uncertain about is this test. I can definitely see that dict(HTTPHeaderDict(baz='quuz')) == {'baz': 'quuz'} but something else is turning that into {'Baz': 'quuz'}. I haven't investigated it yet.

shazow · 2015-07-18T14:08:42Z

Hm wonder if it's our dummyserver and/or httplib mangling headers on some versions.

sigmavirus24 · 2015-07-18T14:14:05Z

Hm wonder if it's our dummyserver and/or httplib mangling headers on some versions.

It's more bizarre because this test was passing before we fixed the round-trip behaviour. =/

sigmavirus24 · 2015-07-18T14:17:37Z

If I debug into the request methods, I get to

(Pdb) p kw['headers']
{'Accept': '*/*', 'baz': 'quux', 'Host': 'localhost:52261'}

On L276 in urllib3/poolmanager.py

sigmavirus24 · 2015-07-18T14:20:01Z

If I go further, it's still the right case in connectionpool.py. I don't have much time to go further though. Sorry. =(

sigmavirus24 · 2015-07-18T14:21:48Z

For anyone else looking to follow the same rabbit hole.

Add import pdb; pdb.set_trace() (or ipdb if that's your thing) to test/with_dummyserver/test_proxy_poolmanager.py at L238.
tox -e py27 -- -s test.with_dummyserver.test_proxy_poolmanager (or py34 or whatever)
Start stepping into functions that make the request.

Lukasa · 2015-07-19T10:21:23Z

So this isn't in httplib: the header is emitted in the case it was provided to httplib. Current money is on the dummyserver doing that.

Lukasa · 2015-07-19T10:23:15Z

Definitely the dummy server. From tcpdump:

GET http://localhost:62953/headers HTTP/1.1
Accept-Encoding: identity
Accept: */*
Host: localhost:62953
baz: quux
foo: bar

HTTP/1.1 200 OK
Etag: "feb15a7ce2766d24cf20cf1633197fc5d8a59281"
Date: Sun, 19 Jul 2015 10:22:50 GMT
Content-Length: 146
Server: TornadoServer/4.1
Content-Type: text/html; charset=UTF-8,text/plain

{"Accept": "*/*", "Content-Length": "0", "Foo": "bar", "Connection": "close", "Host": "localhost:62953", "Accept-Encoding": "gzip", "Baz": "quux"}

We definitely emitted headers in lowercase, they were definitely reflected to us in upper-case.

Lukasa · 2015-07-19T10:28:03Z

Note also that the dummyserver inserts some headers we didn't send (Connection: close) and mangles one we did send (Accept-Encoding: gzip).

Lukasa · 2015-07-19T10:43:28Z

Ok, so had a quick chat with @shazow and I think that test should just be rethought. We can't effectively test that case is preserved when we round-trip via the dummy server, so let's just not.

I can see a case for wanting a test that ensures that case is preserved that includes httplib, but if we're going to do that we should do it as a socket-level test where we can confirm that the bytes-on-the-wire are really what we think they are.

sigmavirus24 · 2015-07-21T01:58:17Z

So b49490d has a socketlevel test for this.

shazow · 2015-07-21T08:28:02Z

Yay 🍮!

Are we good to go? Shall I merge and ship v1.11?

sigmavirus24 · 2015-07-21T12:34:23Z

That's Cory's call.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Lukasa · 2015-07-21T12:35:36Z

Let's rock-and-roll.

Use HTTPHeaderDict in request

mmedvede · 2015-08-05T17:34:42Z

This causes OpenStack devstack-gate tests to fail on Fedora 21 (but not on Ubuntu). There are interactions between many python packages during the test, so I am not yet certain if it is urlib's fault, but if I use the commit previous to the merge of this pull request, tempest tests pass.

Lukasa · 2015-08-05T20:33:06Z

@mmedvede Can we get more information, e.g. a link to a Launchpad bug?

sigmavirus24 · 2015-08-06T03:54:50Z

@Lukasa it appears they're tracking it in https://bugs.launchpad.net/glance/+bug/1476770. The original bug was because the gate is not operating as it is supposed to (and I suspect the same is happening on Fedora). The gate should always be installing requests from pip instead of using system packages but they're not so when they install urllib3 1.11.0, it installs over the system urllib3 that was installed. I maintain that this is fundamentally a problem with the gate. Note that instead of fixing the gate, they released a new version of oslo.vmware (the only library to use urllib3 in openstack directly) with a cap to exclude 1.11.0. This is not a problem or a bug in urllib3.

mmedvede · 2015-08-06T13:40:35Z

@sigmavirus24 I was not aware of the bug, thanks. It is exactly what was happening to our third-party Fedora 21 gate (IBM PowerKVM CI) before I disabled the urllib3 1.11 install. I assume that Ubuntu installs newer system python-requests, so when urllib3 1.11 is installed on top, the gate doesn't break. The weird thing is that both Ubuntu and Fedora gates show requests 2.7.0 installed. I have confirmed that doing 'pip install --upgrade --force-reinstall requests' fixes Fedora 21 in my scenario, even though requests would still show as being 2.7.0.

@Lukasa Thanks for quick response. Not urllib3 problem.

sigmavirus24 · 2015-08-06T13:45:01Z

@mmedvede check where requests 2.7.0 is installed from. You'll see that it's installed from the system packages and that is not what the gate is supposed to do. Anyway, further discussion should happen on LaunchPad, not here.

tfmorris · 2015-11-30T22:56:38Z

This change requires all requests-cache caches be deleted.

shazow and others added 2 commits May 28, 2015 11:25

Tests for HTTPHeaderDict in requests (re: urllib3#632)

838d23a

Change HTTPHeaderDict superclass to MutableMapping

64adf9f

Fix dict(HTTPHeaderDict(other_dict)) and deal with changes in how we implement the HTTPHeaderDict as a MutableMapping subclass instead of as a dict subclass.

sigmavirus24 reviewed Jul 18, 2015
View reviewed changes

Lukasa added this to the v1.11 milestone Jul 19, 2015

shazow mentioned this pull request Jul 19, 2015

HTTPHeaderDict in requests #633

Closed

2 tasks

Add a socketlevel test to ensure case is preserved

b49490d

shazow added a commit that referenced this pull request Jul 21, 2015

Merge pull request #679 from sigmavirus24/httpheaderdict-requests

a76eb3b

Use HTTPHeaderDict in request

shazow merged commit a76eb3b into urllib3:master Jul 21, 2015

shazow mentioned this pull request Jul 21, 2015

Urllib3 HTTPHeaderDict and proxy_from_url problem #632

Closed

shazow added a commit that referenced this pull request Jul 21, 2015

CHANGES for #679

7add2f3

tfmorris mentioned this pull request Nov 30, 2015

TypeError: HTTPHeaderDict is not a subtype of dict requests-cache/requests-cache#56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use HTTPHeaderDict in request #679

Use HTTPHeaderDict in request #679

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 Jul 18, 2015

Lukasa commented Jul 18, 2015

shazow commented Jul 18, 2015

shazow commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

shazow commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

Lukasa commented Jul 19, 2015

Lukasa commented Jul 19, 2015

Lukasa commented Jul 19, 2015

Lukasa commented Jul 19, 2015

sigmavirus24 commented Jul 21, 2015

shazow commented Jul 21, 2015

sigmavirus24 commented Jul 21, 2015

Lukasa commented Jul 21, 2015

mmedvede commented Aug 5, 2015

Lukasa commented Aug 5, 2015

sigmavirus24 commented Aug 6, 2015

mmedvede commented Aug 6, 2015

sigmavirus24 commented Aug 6, 2015

tfmorris commented Nov 30, 2015

Use HTTPHeaderDict in request #679

Use HTTPHeaderDict in request #679

Conversation

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 Jul 18, 2015

Choose a reason for hiding this comment

Lukasa commented Jul 18, 2015

shazow commented Jul 18, 2015

shazow commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

shazow commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

sigmavirus24 commented Jul 18, 2015

Lukasa commented Jul 19, 2015

Lukasa commented Jul 19, 2015

Lukasa commented Jul 19, 2015

Lukasa commented Jul 19, 2015

sigmavirus24 commented Jul 21, 2015

shazow commented Jul 21, 2015

sigmavirus24 commented Jul 21, 2015

That's Cory's call.

Lukasa commented Jul 21, 2015

mmedvede commented Aug 5, 2015

Lukasa commented Aug 5, 2015

sigmavirus24 commented Aug 6, 2015

mmedvede commented Aug 6, 2015

sigmavirus24 commented Aug 6, 2015

tfmorris commented Nov 30, 2015