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

Asyncio reuseport #68160

Closed
Lothiraldan mannequin opened this issue Apr 16, 2015 · 16 comments
Closed

Asyncio reuseport #68160

Lothiraldan mannequin opened this issue Apr 16, 2015 · 16 comments
Assignees
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@Lothiraldan
Copy link
Mannequin

Lothiraldan mannequin commented Apr 16, 2015

BPO 23972
Nosy @gvanrossum, @vstinner, @Lothiraldan, @1st1
Files
  • 23972_cjl.patch: Patch, tests and docs updates for 23972
  • 23972_cjl_v002.patch: Updated patch, tests, docs for 23972
  • 23972_cjl_v003.patch: rebase patch, tests, docs for 23972
  • 23972_cjl_v004.patch: Patch, tests, docs updates for 23972
  • 23972_cjl_v005.patch
  • 23972_cjl_v006.patch: refine tests to also work on Windows and Debian platforms
  • 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/gvanrossum'
    closed_at = <Date 2015-10-05.16:33:28.391>
    created_at = <Date 2015-04-16.11:52:15.669>
    labels = ['type-bug', 'expert-asyncio']
    title = 'Asyncio reuseport'
    updated_at = <Date 2015-10-06.15:25:58.881>
    user = 'https://github.com/Lothiraldan'

    bugs.python.org fields:

    activity = <Date 2015-10-06.15:25:58.881>
    actor = 'python-dev'
    assignee = 'gvanrossum'
    closed = True
    closed_date = <Date 2015-10-05.16:33:28.391>
    closer = 'gvanrossum'
    components = ['asyncio']
    creation = <Date 2015-04-16.11:52:15.669>
    creator = 'Boris.FELD'
    dependencies = []
    files = ['39930', '40230', '40616', '40664', '40667', '40696']
    hgrepos = []
    issue_num = 23972
    keywords = ['patch']
    message_count = 16.0
    messages = ['241213', '241220', '246691', '246768', '246779', '249000', '251242', '251837', '251884', '252189', '252223', '252338', '252339', '252373', '252379', '252396']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'vstinner', 'Boris.FELD', 'python-dev', 'yselivanov', 'ysionneau', 'chris laws', 'j1o1h1n']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23972'
    versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

    @Lothiraldan
    Copy link
    Mannequin Author

    Lothiraldan mannequin commented Apr 16, 2015

    I'm trying to create some UDP sockets for doing multicast communication. I have the working code in synchronous way and try to port it to asyncio.

    One last issue is blocking for me, I'm on Mac OS X and for multicast UDP to work, the SO_REUSEPORT must be set on socket before bind. The problem is that I don't have access on socket, it's created inside asyncio method _create_connection_transport.

    I've seen that SO_REUSEADDR is already set (https://github.com/gvanrossum/tulip-try3/blob/7b2d8abfce1d7ef18ef516f9b1b7032172630375/asyncio/base_events.py#L720), so maybe we could also set SO_REUSEPORT only on platforms where it's available. if hasattr(socket, 'SO_REUSEPORT') should works.

    Or we could add an optional arguments with could be used to set some socket options, it could be more flexible that set SO_REUSEPORT.

    I could provide a patch for the best solution selected.

    @Lothiraldan Lothiraldan mannequin added topic-asyncio type-bug An unexpected behavior, bug, or error labels Apr 16, 2015
    @gvanrossum
    Copy link
    Member

    I think we left this unfinished. I think it would be best if we added a sock parameter (like for create_server()) but also added reuse_address and reuse_port parameters, treated similarly like the one on create_server(), and added a reuse_port parameter to create_server().

    Or maybe there should only be one parameter that guides both; read http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-so-reuseport-how-do-they-differ-do-they-mean-t before you decide!

    @chrislaws
    Copy link
    Mannequin

    chrislaws mannequin commented Jul 13, 2015

    I encountered this issue too. I needed it resolved ASAP for my work so I created a loop patch that partially implements the suggestion solution by overriding the create_datagram_endpoint method. Perhaps this might be of some use to the eventual ticket resolver.

    It can be found in the following gist: https://gist.github.com/claws/d4076b4b155695844d54

    If this looks like it's progressing in the right direction I can try to implement it properly in base_events.py and submit it via a patch.

    @chrislaws
    Copy link
    Mannequin

    chrislaws mannequin commented Jul 15, 2015

    Attached is a patch that implements the suggested solution along with tests and associated doc updates. Hope this helps.

    @vstinner
    Copy link
    Member

    A quick search told me that "Windows only knows the SO_REUSEADDR option, there is no SO_REUSEPORT". It should be at least documented that SO_REUSEADDR is not supported on Windows.

    Maybe we should raise an exception on Windows if reuse_port=True? Ignoring the parameter is a different option, but it should be well documented that the option is ignored if socket.SO_REUSEPORT is not defined.

    @chrislaws
    Copy link
    Mannequin

    chrislaws mannequin commented Aug 23, 2015

    I have updated the patch to address comments raised by haypo.

    An exception is now raised if reuse_port is explicitly used and the platform does not support SOREUSEPORT. The docs have also been updated to make it more explicit that this feature is not supported on Windows.

    @vstinner
    Copy link
    Member

    Hum, the latest patch was not reviewed yet :-(

    @chrislaws
    Copy link
    Mannequin

    chrislaws mannequin commented Sep 29, 2015

    Rebase patch onto current master.

    @gvanrossum
    Copy link
    Member

    I added a whole bunch of review comments. Please send a new patch!

    @chrislaws
    Copy link
    Mannequin

    chrislaws mannequin commented Oct 3, 2015

    Updates addressing review comments.

    @gvanrossum
    Copy link
    Member

    I'm adding a rebased version of Chris's v4 patch to see if I can get the code review to trigger.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 5, 2015

    New changeset 5e7e9b131904 by Guido van Rossum in branch '3.4':
    Issue bpo-23972: updates to asyncio datagram API. By Chris Laws.
    https://hg.python.org/cpython/rev/5e7e9b131904

    New changeset ba956289fe66 by Guido van Rossum in branch '3.5':
    Issue bpo-23972: updates to asyncio datagram API. By Chris Laws. (Merge 3.4->3.5.)
    https://hg.python.org/cpython/rev/ba956289fe66

    New changeset c0f1f882737c by Guido van Rossum in branch 'default':
    Issue bpo-23972: updates to asyncio datagram API. By Chris Laws. (Merge 3.5->3.6.)
    https://hg.python.org/cpython/rev/c0f1f882737c

    @gvanrossum
    Copy link
    Member

    Thanks for the patch! It's live now. Closing. Watch the buildbots for me please!

    @gvanrossum gvanrossum self-assigned this Oct 5, 2015
    @chrislaws
    Copy link
    Mannequin

    chrislaws mannequin commented Oct 6, 2015

    I've checked the Buildbot results and observed a few errors related to this change. Specifically the issues affect Windows and Debian.

    Builder AMD64 Debian root 3.x build bpo-2772 - http://buildbot.python.org/all/builders/AMD64%20Debian%20root%203.x/builds/2772
    Builder AMD64 Windows7 SP1 3.x build bpo-6808 - http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/6808

    In test_create_datagram_endpoint_sock the Windows-based buildbot raises an error related to using sock.getsockname(). This issue has been observed before on Windows using Python.

    Refs:
    https://mail.python.org/pipermail/python-list/2015-January/683777.html
    http://stackoverflow.com/questions/15638214/socket-error-invalid-argument-supplied

    In this test the socket is not really used so I was trying to get away with minimal actions to set up the test. The suggested solution would be to bind the socket before using it in the test.

    sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
    sock.bind(('0.0.0.0', 0))

    In test_create_datagram_endpoint_sockopts we can't use the (else clause at line 1310) because if SO_REUSEPORT is not defined it can not be used in a check. This affects Windows and Debian where SO_REUSEPORT is not defined.

    From:
    if reuseport_supported:
    self.assertTrue(
    sock.getsockopt(
    socket.SOL_SOCKET, socket.SO_REUSEPORT))
    else:
    self.assertFalse(
    sock.getsockopt(
    socket.SOL_SOCKET, socket.SO_REUSEPORT))

    To:

    if reuseport_supported:
        self.assertTrue(
            sock.getsockopt(
                socket.SOL_SOCKET, socket.SO_REUSEPORT))

    I'll submit a patch to resolve these issues later today.

    @chrislaws
    Copy link
    Mannequin

    chrislaws mannequin commented Oct 6, 2015

    This patch contains minor updates to resolve the Buildbot issues observed on the Windows and Debian platforms after the initial bpo-23972 change set was committed.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 6, 2015

    New changeset aebbf205ef6f by Guido van Rossum in branch '3.4':
    Issue bpo-23972: Fix tests for Windows and Debian.
    https://hg.python.org/cpython/rev/aebbf205ef6f

    New changeset 4d643c5df2a5 by Guido van Rossum in branch '3.5':
    Issue bpo-23972: Fix tests for Windows and Debian. (Merge 3.4->3.5)
    https://hg.python.org/cpython/rev/4d643c5df2a5

    New changeset 3e2218a4e629 by Guido van Rossum in branch 'default':
    Issue bpo-23972: Fix tests for Windows and Debian. (Merge 3.5->3.6)
    https://hg.python.org/cpython/rev/3e2218a4e629

    @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
    topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants