Patchwork [STABLE] proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1

login
register
mail settings
Submitter Yuya Nishihara
Date May 22, 2014, 1:55 p.m.
Message ID <b8bca4e9640673f43e90.1400766917@mimosa>
Download mbox | patch
Permalink /patch/4848/
State Accepted
Headers show

Comments

Yuya Nishihara - May 22, 2014, 1:55 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1400763926 -32400
#      Thu May 22 22:05:26 2014 +0900
# Branch stable
# Node ID b8bca4e9640673f43e9027c199c25ff828531fe9
# Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1

With Python 2.7.7rc1, "hg pull" through HTTP CONNECT tunnel fails due to the
removal of _set_hostport [1].

      ...
      File "mercurial/url.py", line 372, in https_open
        return self.do_open(self._makeconnection, req)
      ...
      File "mercurial/url.py", line 342, in connect
        _generic_proxytunnel(self)
      File "mercurial/url.py", line 228, in _generic_proxytunnel
        self._set_hostport(self.host, self.port)
    AttributeError: httpsconnection instance has no attribute '_set_hostport'

self._set_hostport(self.host, self.port) should be noop and can be removed
because:

 - _set_hostport() [2] was the function to parse "host:port" string and
   set them to self.host and self.port,
 - and (self.host, self.port) pair should be valid since connect() is called
   prior to _generic_proxytunnel().

 [1]: http://hg.python.org/cpython/rev/568041fd8090
 [2]: http://hg.python.org/cpython/file/3a1db0d2747e/Lib/httplib.py#l721
Mads Kiilerich - May 22, 2014, 2:03 p.m.
On 05/22/2014 03:55 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1400763926 -32400
> #      Thu May 22 22:05:26 2014 +0900
> # Branch stable
> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9
> # Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1

Which Python versions has this been tested / reviewed with?

Do the test suite have test coverage for this?

/Mads

> With Python 2.7.7rc1, "hg pull" through HTTP CONNECT tunnel fails due to the
> removal of _set_hostport [1].
>
>        ...
>        File "mercurial/url.py", line 372, in https_open
>          return self.do_open(self._makeconnection, req)
>        ...
>        File "mercurial/url.py", line 342, in connect
>          _generic_proxytunnel(self)
>        File "mercurial/url.py", line 228, in _generic_proxytunnel
>          self._set_hostport(self.host, self.port)
>      AttributeError: httpsconnection instance has no attribute '_set_hostport'
>
> self._set_hostport(self.host, self.port) should be noop and can be removed
> because:
>
>   - _set_hostport() [2] was the function to parse "host:port" string and
>     set them to self.host and self.port,
>   - and (self.host, self.port) pair should be valid since connect() is called
>     prior to _generic_proxytunnel().
>
>   [1]: http://hg.python.org/cpython/rev/568041fd8090
>   [2]: http://hg.python.org/cpython/file/3a1db0d2747e/Lib/httplib.py#l721
>
> diff --git a/mercurial/url.py b/mercurial/url.py
> --- a/mercurial/url.py
> +++ b/mercurial/url.py
> @@ -225,7 +225,6 @@ def _generic_proxytunnel(self):
>       proxyheaders = dict(
>               [(x, self.headers[x]) for x in self.headers
>                if x.lower().startswith('proxy-')])
> -    self._set_hostport(self.host, self.port)
>       self.send('CONNECT %s HTTP/1.0\r\n' % self.realhostport)
>       for header in proxyheaders.iteritems():
>           self.send('%s: %s\r\n' % header)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - May 22, 2014, 2:34 p.m.
On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote:
> On 05/22/2014 03:55 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1400763926 -32400
> > #      Thu May 22 22:05:26 2014 +0900
> > # Branch stable
> > # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9
> > # Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
> > proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1
> 
> Which Python versions has this been tested / reviewed with?

Tested with

  - Python 2.7.6 (Debian python2.7 2.7.6-8)
  - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1)

and checked _set_hostport implementation at v2.4, v2.5 and v2.6.

> Do the test suite have test coverage for this?

test-https.t should fail with Python 2.7.7rc1.

Regards,
Mads Kiilerich - May 22, 2014, 3:16 p.m.
On 05/22/2014 04:34 PM, Yuya Nishihara wrote:
> On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote:
>> On 05/22/2014 03:55 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1400763926 -32400
>>> #      Thu May 22 22:05:26 2014 +0900
>>> # Branch stable
>>> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9
>>> # Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
>>> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1
>> Which Python versions has this been tested / reviewed with?
> Tested with
>
>    - Python 2.7.6 (Debian python2.7 2.7.6-8)
>    - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1)
>
> and checked _set_hostport implementation at v2.4, v2.5 and v2.6.

FWIW, it is relatively simple and fast to test different Python versions 
using contrib/Makefile.python .

>> Do the test suite have test coverage for this?
> test-https.t should fail with Python 2.7.7rc1.

Will it also verify that the effect of _set_hostport still is achieved?

/Mads
Yuya Nishihara - May 23, 2014, 2:57 p.m.
On Thu, 22 May 2014 17:16:42 +0200, Mads Kiilerich wrote:
> On 05/22/2014 04:34 PM, Yuya Nishihara wrote:
> > On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote:
> >> On 05/22/2014 03:55 PM, Yuya Nishihara wrote:
> >>> # HG changeset patch
> >>> # User Yuya Nishihara <yuya@tcha.org>
> >>> # Date 1400763926 -32400
> >>> #      Thu May 22 22:05:26 2014 +0900
> >>> # Branch stable
> >>> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9
> >>> # Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
> >>> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1
> >> Which Python versions has this been tested / reviewed with?
> > Tested with
> >
> >    - Python 2.7.6 (Debian python2.7 2.7.6-8)
> >    - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1)
> >
> > and checked _set_hostport implementation at v2.4, v2.5 and v2.6.
> 
> FWIW, it is relatively simple and fast to test different Python versions 
> using contrib/Makefile.python .

Thanks.

"hg clone" through tinyproxy.py worked on Python 2.4.6.  But I couldn't
figure out a good way to write test of HTTP CONNECT tunnel for Python 2.4 or
2.5, without ssl module.

> >> Do the test suite have test coverage for this?
> > test-https.t should fail with Python 2.7.7rc1.
> 
> Will it also verify that the effect of _set_hostport still is achieved?

It isn't easy to answer.  AFAIK, "_set_hostport(self.host, self.port)" is
"self.host, self.port = self.host, self.port", and it seems the result of
_set_hostport isn't used because the connection is already established,
but it might be wrong in a certain case.

Regards,
Mads Kiilerich - May 23, 2014, 3:51 p.m.
On 05/23/2014 04:57 PM, Yuya Nishihara wrote:
> On Thu, 22 May 2014 17:16:42 +0200, Mads Kiilerich wrote:
>> On 05/22/2014 04:34 PM, Yuya Nishihara wrote:
>>> On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote:
>>>> On 05/22/2014 03:55 PM, Yuya Nishihara wrote:
>>>>> # HG changeset patch
>>>>> # User Yuya Nishihara <yuya@tcha.org>
>>>>> # Date 1400763926 -32400
>>>>> #      Thu May 22 22:05:26 2014 +0900
>>>>> # Branch stable
>>>>> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9
>>>>> # Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
>>>>> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1
>>>> Which Python versions has this been tested / reviewed with?
>>> Tested with
>>>
>>>     - Python 2.7.6 (Debian python2.7 2.7.6-8)
>>>     - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1)
>>>
>>> and checked _set_hostport implementation at v2.4, v2.5 and v2.6.
>> FWIW, it is relatively simple and fast to test different Python versions
>> using contrib/Makefile.python .
> Thanks.
>
> "hg clone" through tinyproxy.py worked on Python 2.4.6.  But I couldn't
> figure out a good way to write test of HTTP CONNECT tunnel for Python 2.4 or
> 2.5, without ssl module.

It is probably not possible. The Python http implementation has changed 
a lot over time. Proxys did probably not really work correctly for our 
use before 2.6.3 or something like that.

It seems like the change is fine, then. Assuming Python really want to 
change the implementation in such a way that it breaks other tools. 
(They could argue that we are messing with private methods. But. But. 
But that was necessary ... :-( )

/Mads
Augie Fackler - May 26, 2014, 4:15 p.m.
On Thu, May 22, 2014 at 10:55:17PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1400763926 -32400
> #      Thu May 22 22:05:26 2014 +0900
> # Branch stable
> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9
> # Parent  54d7657d7d1e6a62315eea53f4498657e766bb60
> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1

queued for stable per Mads' review, and as it seems sensible and
well-researched.

>
> With Python 2.7.7rc1, "hg pull" through HTTP CONNECT tunnel fails due to the
> removal of _set_hostport [1].
>
>       ...
>       File "mercurial/url.py", line 372, in https_open
>         return self.do_open(self._makeconnection, req)
>       ...
>       File "mercurial/url.py", line 342, in connect
>         _generic_proxytunnel(self)
>       File "mercurial/url.py", line 228, in _generic_proxytunnel
>         self._set_hostport(self.host, self.port)
>     AttributeError: httpsconnection instance has no attribute '_set_hostport'
>
> self._set_hostport(self.host, self.port) should be noop and can be removed
> because:
>
>  - _set_hostport() [2] was the function to parse "host:port" string and
>    set them to self.host and self.port,
>  - and (self.host, self.port) pair should be valid since connect() is called
>    prior to _generic_proxytunnel().
>
>  [1]: http://hg.python.org/cpython/rev/568041fd8090
>  [2]: http://hg.python.org/cpython/file/3a1db0d2747e/Lib/httplib.py#l721
>
> diff --git a/mercurial/url.py b/mercurial/url.py
> --- a/mercurial/url.py
> +++ b/mercurial/url.py
> @@ -225,7 +225,6 @@ def _generic_proxytunnel(self):
>      proxyheaders = dict(
>              [(x, self.headers[x]) for x in self.headers
>               if x.lower().startswith('proxy-')])
> -    self._set_hostport(self.host, self.port)
>      self.send('CONNECT %s HTTP/1.0\r\n' % self.realhostport)
>      for header in proxyheaders.iteritems():
>          self.send('%s: %s\r\n' % header)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/url.py b/mercurial/url.py
--- a/mercurial/url.py
+++ b/mercurial/url.py
@@ -225,7 +225,6 @@  def _generic_proxytunnel(self):
     proxyheaders = dict(
             [(x, self.headers[x]) for x in self.headers
              if x.lower().startswith('proxy-')])
-    self._set_hostport(self.host, self.port)
     self.send('CONNECT %s HTTP/1.0\r\n' % self.realhostport)
     for header in proxyheaders.iteritems():
         self.send('%s: %s\r\n' % header)