python / cpython

The Python programming language
https://www.python.org
Other
62.17k stars 29.88k forks source link

urllib2 localnet Changed test to lookup IP-address of localhost #66942

Open ee30ec5c-c055-4a33-aa8d-d74056925916 opened 9 years ago

ee30ec5c-c055-4a33-aa8d-d74056925916 commented 9 years ago
BPO 22753
Nosy @ezio-melotti, @bitdancer
Files
  • issue_22753.diff: A patch that does this: localhost_ip = socket.gethostbyname('localhost')
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-bug', 'tests'] title = 'urllib2 localnet Changed test to lookup IP-address of localhost' updated_at = user = 'https://bugs.python.org/hakan' ``` bugs.python.org fields: ```python activity = actor = 'hakan' assignee = 'none' closed = False closed_date = None closer = None components = ['Tests'] creation = creator = 'hakan' dependencies = [] files = ['37053'] hgrepos = [] issue_num = 22753 keywords = ['patch'] message_count = 6.0 messages = ['230156', '230157', '230158', '230451', '230456', '230534'] nosy_count = 5.0 nosy_names = ['ezio.melotti', 'r.david.murray', 'puppet', 'larstiq', 'hakan'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue22753' versions = ['Python 3.5'] ```

    ee30ec5c-c055-4a33-aa8d-d74056925916 commented 9 years ago

    Running this test gave me an error: ./python -m test -v test_urllib2_localnet

    \====================================================================== ERROR: test_proxy_qop_auth_works (test.test_urllib2_localnet.ProxyAuthTests) ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/hakan/kod/cpython/cpython/Lib/urllib/request.py", line 1182, in do_open
        h.request(req.get_method(), req.selector, req.data, headers)
      File "/home/hakan/kod/cpython/cpython/Lib/http/client.py", line 1154, in request
        self._send_request(method, url, body, headers)
      File "/home/hakan/kod/cpython/cpython/Lib/http/client.py", line 1192, in _send_request
        self.endheaders(body)
      File "/home/hakan/kod/cpython/cpython/Lib/http/client.py", line 1150, in endheaders
        self._send_output(message_body)
      File "/home/hakan/kod/cpython/cpython/Lib/http/client.py", line 988, in _send_output
        self.send(msg)
      File "/home/hakan/kod/cpython/cpython/Lib/http/client.py", line 923, in send
        self.connect()
      File "/home/hakan/kod/cpython/cpython/Lib/http/client.py", line 900, in connect
        self.timeout, self.source_address)
      File "/home/hakan/kod/cpython/cpython/Lib/socket.py", line 710, in create_connection
        raise err
      File "/home/hakan/kod/cpython/cpython/Lib/socket.py", line 701, in create_connection
        sock.connect(sa)
    ConnectionRefusedError: [Errno 111] Connection refused

    Changing "localhost" to "127.0.0.1" made the test pass. My friend puppet made a patch for it so it looks up the IP-address.

    ee30ec5c-c055-4a33-aa8d-d74056925916 commented 9 years ago

    Ubuntu Linux 12.04 64bit

    bitdancer commented 9 years ago

    Your description of the solution ("change localhost to 127.0.0.1") does not match the content of the patch. The patch appears to be looking up the IP address associated with 'localhost' and using that. On most systems that would be 127.0.0.1, but perhaps on yours it is not.

    I suspect your system has a configuration problem that has nothing to do with Python, but it isn't clear.

    In any case the patch is incorrect, since one of the tests it is changing is specifically testing 127.0.0.1, from what I can see.

    2aa36587-55a8-4795-8dc2-8666bf3f4987 commented 9 years ago

    What the patch does is replace the hard-coded address 127.0.0.1 for the loopback interface with a one-time lookup. I'd argue the hunk @@ -315,7 +316,7 @@ should be dropped.

    The two arguments that motivate this change are:

    1) The address of the loopback interface is not always 127.0.0.1, binding to it may fail. 2) The tests seem to assume localhost resolves to the same address where the server is running.

    Working on this patch started with the ConnectionRefusedError that Håkan mentioned. Applying the patch made the ConnectionRefusedError go away, but I think we may have solved a different problem and suppressed the real problem as a side effect.

    @r.david.murray: would the patch be acceptable on dropping said hunk, and changing the description to "Do not assume localhost resolves to 127.0.0.1"?

    That still leaves, I think, a hidden bug somewhere in the proxy code that will surface in some corner case.

    bitdancer commented 9 years ago

    The problem is that there are several places in the test suite (and the stdlib itself!) that assume that 127.0.0.1 is a valid loopback address. This is the de-facto Internet standard, and while I know not all systems make 127.0.0.1 a valid loopback and/or do not map localhost to it, I'm not sure this is something it is practical for the stdlib or the test suite to compensate for. (I had this problem with linux-vserver on my buildbots, see bpo-3972). It is true that the test suite in in a couple places assumes that 'localhost' maps to 127.0.0.1, which is an assumption that breaks more often than that of 127.0.0.;1 being a a valid loopback, since it depends on the "correct" configuration of the local host.

    We've got a catch 22: if we assume localhost maps to a valid loopback address, the tests will fail on systems that are not configured correctly, while if we assume that 127.0.0.1 is a valid loopback address but may not be 'localhost', the tests will fail on systems for which 127.0.0.1 is not a valid loopback. Since the stdlib itself assumes that 127.0.0.1 is valid, the former wins. (As far as I know we don't have any open issues against the stdlib for this assumption. If it is problematic on your system, then you should open a separate issue about that so we can consider it.)

    If this test fails on your system because 127.0.0.1 is not a loopback address, I would expect many other tests to fail as well (ex: test_ssl). If they do not, then I suspect your real problem is that localhost doesn't map to 127.0.0.1 even though 127.0.0.1 is a valid loopback address. So, it looks to me like the better "fix" here would be to change 'URL' to be 'http://127.0.0.1'. Can you confirm if that works for you, please?

    ee30ec5c-c055-4a33-aa8d-d74056925916 commented 9 years ago

    Yes, I can confirm that changing 'URL' to be 'http://127.0.0.1' works. I think that would be a solution to this.