Issue 6506074: Making httplib2.Http instances pickleable. (Closed)
|
Created:
11 years, 7 months ago by dhermes Modified:
11 years, 4 months ago CC:
httplib2-dev_googlegroups.com Visibility:
Public. |
DescriptionMaking httplib2.Http instances pickleable.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding unit tests for pickled Http objects. #
Total comments: 2
Patch Set 3 : Making sure connections gets dropped and making tests less brittle. #
Total comments: 2
Patch Set 4 : Adding in missing args and kwargs to ResponseDict. #
Total comments: 2
MessagesTotal messages: 18
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#ne... python2/httplib2/__init__.py:1242: # credentials which handle auth So are you planning on reinstating the credentials object at a higher level?
Sign in to reply to this message.
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#ne... 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
Sign in to reply to this message.
Any issue here? On Wed, Sep 5, 2012 at 8:23 AM, <dhermes@google.com> wrote: > > https://codereview.appspot.**com/6506074/diff/1/python2/** > httplib2/__init__.py<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<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/<https://codereview.appspot.com/6506... > -- Daniel Hermes Developer Programs Engineer
Sign in to reply to this message.
Yes, it still needs unit tests like I mentioned in my first email. -joe On Thu, Sep 6, 2012 at 4:34 PM, Daniel Hermes <dhermes@google.com> wrote: > Any issue here? > > > On Wed, Sep 5, 2012 at 8:23 AM, <dhermes@google.com> wrote: >> >> >> 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#ne... >> 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 > Developer Programs Engineer
Sign in to reply to this message.
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? On Thu, Sep 6, 2012 at 6:21 PM, Joe Gregorio <jcgregorio@google.com> wrote: > Yes, it still needs unit tests like I mentioned in my first email. > > -joe > > On Thu, Sep 6, 2012 at 4:34 PM, Daniel Hermes <dhermes@google.com> wrote: > > Any issue here? > > > > > > On Wed, Sep 5, 2012 at 8:23 AM, <dhermes@google.com> wrote: > >> > >> > >> > 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#ne... > >> 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 > > Developer Programs Engineer > -- Daniel Hermes Developer Programs Engineer
Sign in to reply to this message.
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#newc... 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.
Sign in to reply to this message.
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#newc... 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. On 2012/09/13 11:30:33, jcgregorio_google wrote: > 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.
Sign in to reply to this message.
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... python2/httplib2/__init__.py:1075: return super(ResponseDict, self).__init__(*args, **kwargs) Where do *args, and **kwargs come from?
Sign in to reply to this message.
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... 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). On 2012/09/13 16:12:13, jcgregorio_google wrote: > Where do *args, and **kwargs come from?
Sign in to reply to this message.
Joe, Have you had a chance to see the latest diffs? On Thu, Sep 13, 2012 at 9:39 AM, <dhermes@google.com> wrote: > > https://codereview.appspot.**com/6506074/diff/9001/python2/** > httplib2/__init__.py<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<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). > > > On 2012/09/13 16:12:13, jcgregorio_google wrote: > >> Where do *args, and **kwargs come from? >> > > https://codereview.appspot.**com/6506074/<https://codereview.appspot.com/6506... > -- Daniel Hermes Developer Programs Engineer
Sign in to reply to this message.
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__.p... 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?
Sign in to reply to this message.
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__.p... 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. On 2012/09/17 17:46:22, jcgregorio_google wrote: > 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?
Sign in to reply to this message.
LGTM Conditional on getting that next CL to fix up python3 :) On 2012/09/17 17:48:19, dhermes wrote: > 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__.p... > 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. > > On 2012/09/17 17:46:22, jcgregorio_google wrote: > > 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?
Sign in to reply to this message.
Are you sure you are sync'd to head? I get failed hunks when I try to apply this. On 2012/09/18 14:00:33, jcgregorio_google wrote: > LGTM > > Conditional on getting that next CL to fix up python3 :) > > On 2012/09/17 17:48:19, dhermes wrote: > > 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__.p... > > 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. > > > > On 2012/09/17 17:46:22, jcgregorio_google wrote: > > > 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?
Sign in to reply to this message.
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. On Tue, Sep 18, 2012 at 7:04 AM, <jcgregorio@google.com> wrote: > Are you sure you are sync'd to head? I get failed hunks > when I try to apply this. > > > On 2012/09/18 14:00:33, jcgregorio_google wrote: > >> LGTM >> > > Conditional on getting that next CL to fix up python3 :) >> > > On 2012/09/17 17:48:19, dhermes wrote: >> > >> > https://codereview.appspot.**com/6506074/diff/14002/** > python3/httplib2/__init__.py<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<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. >> > >> > On 2012/09/17 17:46:22, jcgregorio_google wrote: >> > > 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/<https://codereview.appspot.com/6506... > -- Daniel Hermes Developer Programs Engineer
Sign in to reply to this message.
Does this work? On Tue, Sep 18, 2012 at 8:30 AM, Daniel Hermes <dhermes@google.com> wrote: > 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. > > > On Tue, Sep 18, 2012 at 7:04 AM, <jcgregorio@google.com> wrote: > >> Are you sure you are sync'd to head? I get failed hunks >> when I try to apply this. >> >> >> On 2012/09/18 14:00:33, jcgregorio_google wrote: >> >>> LGTM >>> >> >> Conditional on getting that next CL to fix up python3 :) >>> >> >> On 2012/09/17 17:48:19, dhermes wrote: >>> > >>> >> https://codereview.appspot.**com/6506074/diff/14002/** >> python3/httplib2/__init__.py<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<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. >>> > >>> > On 2012/09/17 17:46:22, jcgregorio_google wrote: >>> > > 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/<https://codereview.appspot.com/6506... >> > > > > -- > Daniel Hermes > Developer Programs Engineer > -- Daniel Hermes Developer Programs Engineer
Sign in to reply to this message.
Committed at http://code.google.com/p/httplib2/source/detail?r=1f1f7d61957691b482eea7b5c19... On 2012/09/20 21:44:16, dhermes wrote: > Does this work? > > > On Tue, Sep 18, 2012 at 8:30 AM, Daniel Hermes <mailto:dhermes@google.com> wrote: > > > 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. > > > > > > On Tue, Sep 18, 2012 at 7:04 AM, <mailto:jcgregorio@google.com> wrote: > > > >> Are you sure you are sync'd to head? I get failed hunks > >> when I try to apply this. > >> > >> > >> On 2012/09/18 14:00:33, jcgregorio_google wrote: > >> > >>> LGTM > >>> > >> > >> Conditional on getting that next CL to fix up python3 :) > >>> > >> > >> On 2012/09/17 17:48:19, dhermes wrote: > >>> > > >>> > >> https://codereview.appspot.**com/6506074/diff/14002/** > >> > python3/httplib2/__init__.py<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<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. > >>> > > >>> > On 2012/09/17 17:46:22, jcgregorio_google wrote: > >>> > > 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/%3Chttps://codereview.appspot.com/65...> > >> > > > > > > > > -- > > Daniel Hermes > > Developer Programs Engineer > > > > > > -- > Daniel Hermes > Developer Programs Engineer
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
