Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test suite intentionally avoids referring to localhost, destroying abstraction away from IPv6 vs IPv4 #73825

Closed
gpshead opened this issue Feb 24, 2017 · 11 comments
Assignees
Labels
3.7 (EOL) end of life tests Tests in the Lib/test dir

Comments

@gpshead
Copy link
Member

gpshead commented Feb 24, 2017

BPO 29639
Nosy @gpshead, @pfmoore, @pitrou, @vstinner, @tjguk, @bitdancer, @briancurtin, @zware, @zooba
PRs
  • bpo-29639: change test.support.HOST, add HOSTv4 #3465
  • 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:

    assignee = 'https://github.com/gpshead'
    closed_at = <Date 2021-05-19.01:22:22.636>
    created_at = <Date 2017-02-24.01:34:57.135>
    labels = ['3.7', 'tests']
    title = 'test suite intentionally avoids referring to localhost, destroying abstraction away from IPv6 vs IPv4'
    updated_at = <Date 2021-05-19.01:22:22.635>
    user = 'https://github.com/gpshead'

    bugs.python.org fields:

    activity = <Date 2021-05-19.01:22:22.635>
    actor = 'gregory.p.smith'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2021-05-19.01:22:22.636>
    closer = 'gregory.p.smith'
    components = ['Tests']
    creation = <Date 2017-02-24.01:34:57.135>
    creator = 'gregory.p.smith'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29639
    keywords = ['patch']
    message_count = 11.0
    messages = ['288498', '288628', '288638', '288640', '288652', '288682', '301763', '301787', '302087', '302090', '393914']
    nosy_count = 9.0
    nosy_names = ['gregory.p.smith', 'paul.moore', 'pitrou', 'vstinner', 'tim.golden', 'r.david.murray', 'brian.curtin', 'zach.ware', 'steve.dower']
    pr_nums = ['3465']
    priority = 'normal'
    resolution = 'out of date'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue29639'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']

    @gpshead
    Copy link
    Member Author

    gpshead commented Feb 24, 2017

    I am working on fixing our test suite to run on IPv6 only hosts (which are becoming a reality). Many failures today occur because of hard coded 127.0.0.1 values.

    This is wrong. We should refer to "localhost"

    The "solution" to https://bugs.python.org/issue18792 moved us backwards towards hard coding IP version type specific addresses claiming that windows cannot handle resolving localhost reliably.

    On any windows system where that is the case we should declare the system broken and simply not run any networking related tests.

    @gpshead gpshead added 3.7 (EOL) end of life tests Tests in the Lib/test dir labels Feb 24, 2017
    @pitrou
    Copy link
    Member

    pitrou commented Feb 27, 2017

    I'm not sure how much of the original analysis was right. I've just fired up a network-less Windows VM and 'localhost' seems to resolve just fine:

    >>> socket.gethostbyname('localhost')
    '127.0.0.1'
    >>> socket.getaddrinfo('localhost', 80, socket.AF_UNSPEC, socket.SOCK_STREAM)

    [(<AddressFamily.AF_INET6: 23>,
    <SocketKind.SOCK_STREAM: 1>,
    0,
    '',
    ('::1', 80, 0, 0)),
    (<AddressFamily.AF_INET: 2>,
    <SocketKind.SOCK_STREAM: 1>,
    0,
    '',
    ('127.0.0.1', 80))]

    But we should defer to our Windows experts on this.

    (also, perhaps we should simply mandate that buildbots have at least basic DNS functionality. This would lighten the maintenance load on the test suite slightly.)

    @pfmoore
    Copy link
    Member

    pfmoore commented Feb 27, 2017

    I have a vague recollection of once working on a (Windows) system that mis-resolved localhost. But it was a long time ago, and I'm 100% OK with calling such a system broken.

    +1 on using localhost

    @briancurtin
    Copy link
    Member

    I echo Paul. I think the last time I would have seen a problem was on Windows 2000, which is unsupported per PEP-11.

    +1 to using localhost

    @zooba
    Copy link
    Member

    zooba commented Feb 27, 2017

    As far as I recall, there's a hosts file that resolves localhost to 127.0.0.1 on Windows, which means a user could break their own configuration if they so desired. Definitely on all supported versions we should be able to assume localhost can be resolved.

    I haven't checked out how it deals with IPv6, but presumably there's a priority or another hosts file that will cover it.

    @gpshead
    Copy link
    Member Author

    gpshead commented Feb 27, 2017

    great! that makes me feel much less bad about fixing this in the way i desire. :)

    @gpshead gpshead self-assigned this Mar 16, 2017
    @gpshead
    Copy link
    Member Author

    gpshead commented Sep 9, 2017

    New changeset efb1d0a by Gregory P. Smith in branch 'master':
    bpo-29639: change test.support.HOST to "localhost"
    efb1d0a

    @bitdancer
    Copy link
    Member

    Users on linux can and do screw this up too. I believe we also had a case where a distro screwed up the defaults for, I think, the reverse resolve? Not sure which test that was, and the test may since been fixed to not depend on that. The point is this may break in unexpected ways but hopefully they will all be either fixable or legitimately "your system is broken".

    I'll review the test_smtplib changes soonish (so ping me if I don't :)

    @vstinner
    Copy link
    Member

    I would like to share a short story with you.

    I'm working on fixing *all* bugs on our 3 CI (buildbots, Travis CI, AppVeyor). I fixed almost all random test failures.

    Right now, I'm trying to fix all "dangling thread" warnings: bpo-31234.

    I was sure that I was done, but no, test_ssl failed on Travis CI and AppVeyor. Hum. The failure doesn't make sense. The code is perfectly fine. The thread is supposed to be gone for a long time, but not, it's still here for some reason.

    After one day of debugging, I found that the thread is kept alive by a variable of a frame. The frame is kept alive from an traceback object of an Exception. The exception is ConnectionRefusedError. I continue to follow links, I see that the exception comes from socket.create_connection()... Interesting.

    socket.create_connection() tries to be nice and keeps the last exception to re-raise it if no connection succeed.

    The code seems correct: it stores the exception in the variable "err", and "return sock" is used to exit on succeed.

    *But*.

    It seems like the exception stored in "err" is part of a reference cycle, so indirectly, a lot of frames are kept alive because of this cycle.

    So, I wanted to share this story with you because test_ssl only started to fail recently. The reason is that support.HOST was modified from "127.0.0.1" to "localhost". So if the name resolution first returns an IPv6 address, we may get the ConnectionRefusedError error, stored in "err", and then the connection succeed with IPv4... but you get the reference cycle mess.

    Modifying support.HOST to "localhost" triggered a reference cycle!? Strange story.

    I'm working on a quick fix: #3546

    @gpshead
    Copy link
    Member Author

    gpshead commented Sep 13, 2017

    LOL. That is a very strange story and the last thing i'd have expected to fall out from changing one string to another. :)

    @gpshead
    Copy link
    Member Author

    gpshead commented May 19, 2021

    I'm making progress on https://bugs.python.org/issue37901 and related IPv6-only issues, the changes of which tend to shake out improper uses of 127.0.0.1 hardcoded.

    I'm closing this issue as there aren't specific needs that the rest won't cover.

    @gpshead gpshead closed this as completed May 19, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life tests Tests in the Lib/test dir
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants