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

DatagramProtocol + IPv6 does not work with ProactorEventLoop #83329

Closed
agronholm mannequin opened this issue Dec 28, 2019 · 20 comments
Closed

DatagramProtocol + IPv6 does not work with ProactorEventLoop #83329

agronholm mannequin opened this issue Dec 28, 2019 · 20 comments
Labels
3.8 only security fixes 3.9 only security fixes OS-windows topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@agronholm
Copy link
Mannequin

agronholm mannequin commented Dec 28, 2019

BPO 39148
Nosy @pfmoore, @tjguk, @njsmith, @asvetlov, @agronholm, @zware, @1st1, @zooba, @aixtools, @miss-islington, @afflux
PRs
  • bpo-39148: enable ipv6 for datagrams in Proactor #19121
  • [3.8] bpo-39148: enable ipv6 for datagrams in Proactor (GH-19121) #20169
  • bpo-39148: fixup to account for IPV6_ENABLED being moved #20170
  • Files
  • udpreceive.py: Script to reproduce the error
  • 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 = None
    closed_at = None
    created_at = <Date 2019-12-28.10:55:11.585>
    labels = ['type-bug', '3.8', '3.9', 'OS-windows', 'expert-asyncio']
    title = 'DatagramProtocol + IPv6 does not work with ProactorEventLoop'
    updated_at = <Date 2020-05-18.10:52:49.519>
    user = 'https://github.com/agronholm'

    bugs.python.org fields:

    activity = <Date 2020-05-18.10:52:49.519>
    actor = 'Michael.Felt'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Windows', 'asyncio']
    creation = <Date 2019-12-28.10:55:11.585>
    creator = 'alex.gronholm'
    dependencies = []
    files = ['48805']
    hgrepos = []
    issue_num = 39148
    keywords = ['patch']
    message_count = 20.0
    messages = ['358940', '364803', '364807', '364837', '364841', '364842', '364861', '365813', '366934', '367096', '367098', '367174', '367177', '369107', '369185', '369186', '369187', '369192', '369193', '369205']
    nosy_count = 12.0
    nosy_names = ['paul.moore', 'tim.golden', 'njs', 'asvetlov', 'alex.gronholm', 'zach.ware', 'yselivanov', 'steve.dower', 'Michael.Felt', 'miss-islington', 'kbr', 'Marcin Wi\xc5\x9bniewsk']
    pr_nums = ['19121', '20169', '20170']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue39148'
    versions = ['Python 3.8', 'Python 3.9']

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Dec 28, 2019

    Receiving a UDP datagram using DatagramProtocol on the Proactor event loop results in error_received() being called with WinError 87 (Invalid Parameter). The low-level sock_recv() works fine, but naturally loses the sender address information. The attached script works fine as-is on Linux, and on Windows if ::1 is replaced with 127.0.0.1.

    There were extensive tests added for UDP support on IOCP, but unfortunately all of them use only IPv4 sockets so they could not catch this problem.

    @agronholm agronholm mannequin added 3.8 only security fixes OS-windows type-bug An unexpected behavior, bug, or error topic-asyncio labels Dec 28, 2019
    @afflux
    Copy link
    Mannequin

    afflux mannequin commented Mar 22, 2020

    I tried some debugging with Python 3.9.0a4 and it looks like unparse_address in overlapped.c (https://github.com/python/cpython/blob/v3.9.0a4/Modules/overlapped.c#L689) raises the error. Almost seems like ENABLE_IPV6 is not set...

    @afflux
    Copy link
    Mannequin

    afflux mannequin commented Mar 22, 2020

    Just to confirm, for some reason ENABLE_IPV6 is not set when compiling _overlapped. I'm not familiar enough with the windows build infrastructure to submit a PR though.

    @afflux afflux mannequin added 3.9 only security fixes labels Mar 22, 2020
    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Mar 23, 2020

    Well, I found this:

    cpython/configure.ac

    Lines 3278 to 3279 in 3c97e1e

    AH_TEMPLATE(ENABLE_IPV6, [Define if --enable-ipv6 is specified])
    AC_MSG_CHECKING([if --enable-ipv6 is specified])

    Could it be that the official binaries were compiled without --enable-ipv6?

    @afflux
    Copy link
    Mannequin

    afflux mannequin commented Mar 23, 2020

    ./configure is not run on Windows, but I believe the define should go here:

    #define HAVE_INET_PTON 1

    @asvetlov
    Copy link
    Contributor

    Would somebody be a champion for the issue?

    @zooba
    Copy link
    Member

    zooba commented Mar 23, 2020

    Main thing we need is a PR (straightforward) and confirming that the relevant tests all run. We can use the buildbot fleet to make sure that all supported platforms have the necessary APIs, but it wouldn't surprise me if there's some weird edge cases here.

    @afflux
    Copy link
    Mannequin

    afflux mannequin commented Apr 5, 2020

    PR is up. Any chance we get this reviewed for inclusion in 3.9.0a6 / 3.8.3rc1?

    @afflux
    Copy link
    Mannequin

    afflux mannequin commented Apr 21, 2020

    Please let me know if there is anything else I can do to get this going.

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Apr 23, 2020

    Which PR is it?

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Apr 23, 2020

    Oh, it's #19121. I think it would be prudent to add a test as well to make sure this doesn't happen again.

    @afflux
    Copy link
    Mannequin

    afflux mannequin commented Apr 24, 2020

    Fair enough, PR updated.

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented Apr 24, 2020

    Thanks, looks good to me now!

    @agronholm
    Copy link
    Mannequin Author

    agronholm mannequin commented May 17, 2020

    The PR is still awaiting for a core developer to be reviewed. It's too bad we missed the 3.8.3 window, but perhaps this can get included in 3.9.0 at least.

    @miss-islington
    Copy link
    Contributor

    New changeset 442634c by Kjell Braden in branch 'master':
    bpo-39148: enable ipv6 for datagrams in Proactor (GH-19121)
    442634c

    @miss-islington
    Copy link
    Contributor

    New changeset 94d9c5e by Miss Islington (bot) in branch '3.8':
    bpo-39148: enable ipv6 for datagrams in Proactor (GH-19121)
    94d9c5e

    @aixtools
    Copy link
    Contributor

    Bot failed for AIX https://buildbot.python.org/all/#builders/227/builds/978 with:

    0:07:11 Re-running test_asyncio in verbose mode
    Failed to import test module: test.test_asyncio.test_events
    Traceback (most recent call last):
      File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/unittest/loader.py", line 436, in _find_test_path
        module = self._get_module_from_name(name)
      File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/unittest/loader.py", line 377, in _get_module_from_name
        __import__(name)
      File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/test_asyncio/test_events.py", line 239, in <module>
        class EventLoopTestsMixin:
      File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/test_asyncio/test_events.py", line 1262, in EventLoopTestsMixin
        @unittest.skipUnless(support.IPV6_ENABLED, 'IPv6 not supported or enabled')
    AttributeError: module 'test.support' has no attribute 'IPV6_ENABLED'
    test test_asyncio crashed -- Traceback (most recent call last):
      File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/libregrtest/runtest.py", line 270, in _runtest_inner
        refleak = _runtest_inner2(ns, test_name)
      File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/libregrtest/runtest.py", line 234, in _runtest_inner2
        test_runner()
      File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/libregrtest/runtest.py", line 208, in _test_module
        raise Exception("errors while loading tests")
    Exception: errors while loading tests
    1 test failed again:
        test_asyncio

    @njsmith
    Copy link
    Contributor

    njsmith commented May 18, 2020

    New changeset 58205a0 by Nathaniel J. Smith in branch 'master':
    bpo-39148: fixup to account for IPV6_ENABLED being moved (GH-20170)
    58205a0

    @njsmith
    Copy link
    Contributor

    njsmith commented May 18, 2020

    I think I fixed the buildbot issues in #64369, but I can't seem to reach the buildbot site right now, so it's hard to know for sure!

    @aixtools
    Copy link
    Contributor

    I could not "fathom" the buildbot test results - however, a manual build of PR29170 on 3.9 works: I'll try 3.8, but then from master (assuming it is already part of master) -- and that works as well.

    Thanks for the quick update!

    @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.8 only security fixes 3.9 only security fixes OS-windows topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants