Making httplib2.Http instances pickleable. (issue 6506074)

[dhe...@google.com]

Reviewers: jcgregorio,

Description:
Making httplib2.Http instances pickleable.

Please review this at http://codereview.appspot.com/6506074/

Affected files:
M python2/httplib2/__init__.py
M python2/httplib2test.py
M python3/httplib2/__init__.py

[jcgre...@google.com]

Needs unit tests, including one where you attach an attribute to the
request method.


https://codereview.appspot.com/6506074/diff/1/python2/httplib2/__init__.py
File python2/httplib2/__init__.py (right):

https://codereview.appspot.com/6506074/diff/1/python2/httplib2/__init__.py#newcode1242
python2/httplib2/__init__.py:1242: # credentials which handle auth
So are you planning on reinstating the credentials object at a higher
level?

https://codereview.appspot.com/6506074/

[dhe...@google.com]

https://codereview.appspot.com/6506074/diff/1/python2/httplib2/__init__.py
File python2/httplib2/__init__.py (right):

https://codereview.appspot.com/6506074/diff/1/python2/httplib2/__init__.py#newcode1242
python2/httplib2/__init__.py:1242: # credentials which handle auth

On 2012/09/05 15:20:07, jcgregorio_google wrote:
> So are you planning on reinstating the credentials object at a higher
level?

Yes. Given that the request attribute could be wrapped by multiple calls
such as credentials.authorize, it would not be possible to do this in
__setstate__ without
1) recording each such action in order
2) ensuring the caller that wraps the request method is pickleable

https://codereview.appspot.com/6506074/

[Daniel Hermes]

Any issue here?

--
Daniel Hermes
Developer Programs Engineer

[Joe Gregorio]

Yes, it still needs unit tests like I mentioned in my first email.

-joe

[Daniel Hermes]

Sorry it took me 6 days.

I have added unit tests, though I'm not sure how consistent pickle values are (since ordering in a dict is not always guaranteed). I'm not entirely sure what to test in the typical case.

Have a look at the new unit tests testPickleHttp and testPickleCustomRequestHttp in HttpTest for both python2 and python3.

Also, I pulled the latest changes from the repo and for some reason rietveld thinks one of them was made by me. Do I need to change the relative commit somewhere?

[jcgre...@google.com]

https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py
File python2/httplib2test.py (right):

https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py#newcode44
python2/httplib2test.py:44: pickleTemplate = (
This is going to be fragile. For the test just pickle and unpickle and
Http() instance and test that properties haven't changed. I'm
particularly interested that the cache of open connection objects gets
cleared.

https://codereview.appspot.com/6506074/

[dhe...@google.com]

https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py
File python2/httplib2test.py (right):

https://codereview.appspot.com/6506074/diff/6001/python2/httplib2test.py#newcode44
python2/httplib2test.py:44: pickleTemplate = (

I got rid of the fragility and used (object).__dict__.

Connection objects weren't getting cleared by default, but I added this
behavior in __getstate__ and added tests for it.

[jcgre...@google.com]

https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py
File python2/httplib2/__init__.py (right):

https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py#newcode1075
python2/httplib2/__init__.py:1075: return super(ResponseDict,
self).__init__(*args, **kwargs)
Where do *args, and **kwargs come from?

https://codereview.appspot.com/6506074/

[dhe...@google.com]

https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py
File python2/httplib2/__init__.py (right):

https://codereview.appspot.com/6506074/diff/9001/python2/httplib2/__init__.py#newcode1075
python2/httplib2/__init__.py:1075: return super(ResponseDict,
self).__init__(*args, **kwargs)

Good catch. I must have accidentally left it out when I had to reformat
with delete-trailing-whitespace turned off.

They were meant to just allow the constructor to dict to behave
normally, save a possibility of content being passed in (but defaulting
to None otherwise).

[Daniel Hermes]

Joe,

Have you had a chance to see the latest diffs?

[jcgre...@google.com]

https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py
File python3/httplib2/__init__.py (right):

https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py#newcode801
python3/httplib2/__init__.py:801: def __init__(self, cache=None,
timeout=None, proxy_info=None,
Looks like defaulting to proxyinfo from environment never made it into
the code, even though that's what the doc comments say. Can you update
the proxy handling for Python 3 be the same as Python 2?

https://codereview.appspot.com/6506074/

[dhe...@google.com]

https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py
File python3/httplib2/__init__.py (right):

https://codereview.appspot.com/6506074/diff/14002/python3/httplib2/__init__.py#newcode801
python3/httplib2/__init__.py:801: def __init__(self, cache=None,
timeout=None, proxy_info=None,

Joe,

This never existed before. Can we add this in a subsequent change? The
point of this change was to make Http instances pickle-able.

I'll be happy to start working on the next change right away.

[jcgre...@google.com]

LGTM

Conditional on getting that next CL to fix up python3 :)

[jcgre...@google.com]

Are you sure you are sync'd to head? I get failed hunks
when I try to apply this.

[Daniel Hermes]

I did sync to head. I am also getting failed hunks when I try to apply to a clean repo, but it is unclear why they are failing.

The first hunk is an "import pickle" statement that falls in alphabetical order in python3/httplib2test.py and the second is just the tests I added.

I don't know enough about mercurial/rietveld to understand why this is failing. I manually applied the failed hunks and ran an "hg diff > manual_apply.diff" and have attached that diff, which will successfully apply.

Let me know if that works for you.

[Daniel Hermes]

Does this work?