Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#3059 defect closed fixed (fixed)

twisted.internet.tcp.Client.getPeer incorrectly returns hostnames

Reported by: Jean-Paul Calderone Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: Branch: branches/getpeer-hostname-3059-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

If a hostname is passed to connectTCP, it will appear in the peer address. The IPv4Address should always have the IP address instead.

The IConnector returned by connectTCP seems to have the same problem with its getDestination method. This is a bit trickier, since the connector is available synchronously, whereas name resolution is asynchronous...

Change History (16)

comment:1 Changed 9 years ago by Glyph

This highlights another deficiency in Connector as well; its job should be reserved for something like endpoints.

If getDestination is supposed to answer the question "where will this connector reconnect to" then it is fundamentally broken. Passing a hostname to connectTCP and then calling connect on the connector ought to reconnect to the hostname, including name resolution (honoring, for example, round-robin DNS if it is configured for that host).

Perhaps we should simply remove that method, or give it a different return type if a hostname was specified? (Returning an IPv4Address with a hostname in it is definitely wrong, no argument there.)

comment:2 Changed 9 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/getpeer-hostname-3059

(In [23993]) Branching to 'getpeer-hostname-3059'

comment:3 Changed 9 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Glyph deleted

Client.getPeer fixed

comment:4 Changed 9 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Please merge.

comment:5 Changed 9 years ago by therve

Oups, this fails under iocpreactor:

[FAIL]: twisted.internet.test.test_tcp.TCPClientTestsBuilder_IOCPReactor.test_addresses

Traceback (most recent call last):
  File "s:\buildbots\twisted\server-2k8-x86-py2.5-select\Twisted\twisted\internet\test\test_tcp.py", line 111, in test_addresses
    IPv4Address('TCP', '127.0.0.1', serverAddress.port))
twisted.trial.unittest.FailTest: not equal:
a = <twisted.internet.address.IPv4Address object at 0x01C7CE70>
b = <twisted.internet.address.IPv4Address object at 0x01C58270>

I guess this is because of useless code duplication :/

comment:6 Changed 9 years ago by Jean-Paul Calderone

(In [24055]) realAddress

refs #3059

comment:7 Changed 9 years ago by Jean-Paul Calderone

comment:8 Changed 9 years ago by Jean-Paul Calderone

Branch: branches/getpeer-hostname-3059branches/getpeer-hostname-3059-2

(In [24057]) Branching to 'getpeer-hostname-3059-2'

comment:9 Changed 9 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Okay, that build failed. More recent results - <http://buildbot.twistedmatrix.com/builders/server-2k8-x86-py2.5-iocp/builds/88>, <http://buildbot.twistedmatrix.com/builders/winxp32-py2.5-iocp/builds/117>, including a fix to correctly propagate the right reactor in tcp.py that I pulled out of the #3218 branch.

comment:10 Changed 9 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

Among other things:

===============================================================================
[ERROR]: twisted.test.test_unix.UnixSocketTestCase.test_pidFile

Traceback (most recent call last):
  File "/home/glyph/Projects/Twisted/trunk/twisted/internet/tcp.py", line 834, in doRead
    transport = self.transport(skt, protocol, addr, self, s, self.reactor)
exceptions.TypeError: __init__() takes exactly 6 arguments (7 given)

comment:11 Changed 9 years ago by Jean-Paul Calderone

(In [24089]) funny, I forgot to do this last time, too

refs #3059

comment:12 Changed 9 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

comment:13 Changed 9 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Please merge.

comment:14 Changed 9 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [24132]) Merge getpeer-hostname-3059-2

Author: exarkun Reviewer: glyph, therve Fixes: #3059

Fix the implementations of getPeer for client transports so that if the connection was made using a hostname, the returned IPv4Address still contains an IPv4 address instead of the hostname.

comment:15 Changed 7 years ago by Jean-Paul Calderone

This was a duplicate of #2738.

comment:16 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.