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

dup_socket() on Windows should use WSA_FLAG_OVERLAPPED #58508

Closed
sbt mannequin opened this issue Mar 14, 2012 · 8 comments
Closed

dup_socket() on Windows should use WSA_FLAG_OVERLAPPED #58508

sbt mannequin opened this issue Mar 14, 2012 · 8 comments

Comments

@sbt
Copy link
Mannequin

sbt mannequin commented Mar 14, 2012

BPO 14300
Nosy @amauryfa, @pitrou, @kristjanvalur
Files
  • socket_dup.patch
  • 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 = <Date 2012-03-31.23:26:04.820>
    created_at = <Date 2012-03-14.12:59:45.898>
    labels = []
    title = 'dup_socket() on Windows should use WSA_FLAG_OVERLAPPED'
    updated_at = <Date 2012-04-01.20:15:32.295>
    user = 'https://bugs.python.org/sbt'

    bugs.python.org fields:

    activity = <Date 2012-04-01.20:15:32.295>
    actor = 'kristjan.jonsson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-03-31.23:26:04.820>
    closer = 'pitrou'
    components = []
    creation = <Date 2012-03-14.12:59:45.898>
    creator = 'sbt'
    dependencies = []
    files = ['24841']
    hgrepos = []
    issue_num = 14300
    keywords = ['patch']
    message_count = 8.0
    messages = ['155748', '155749', '155750', '155752', '157239', '157240', '157326', '157328']
    nosy_count = 5.0
    nosy_names = ['amaury.forgeotdarc', 'pitrou', 'kristjan.jonsson', 'python-dev', 'sbt']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue14300'
    versions = ['Python 3.3']

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Mar 14, 2012

    According to Microsoft's documentation sockets created using socket() have the
    overlapped attribute, but sockets created with WSASocket() do not unless you
    pass the WSA_FLAG_OVERLAPPED flag. The documentation for WSADuplicateSocket()
    says

    If the source process uses the socket function to create the socket, the
    destination process must pass the WSA_FLAG_OVERLAPPED flag to its WSASocket
    function call.

    This means that dup_socket() in socketmodule.c should use

        return WSASocket(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO,
                         FROM_PROTOCOL_INFO, &info, 0, WSA_FLAG_OVERLAPPED);

    instead of

        return WSASocket(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO,
                         FROM_PROTOCOL_INFO, &info, 0, 0);

    (On Windows, the new multiprocessing.connection.wait() function depends on
    the overlapped attribute, although it is primarily intended for use with pipe
    connections not sockets.)

    Patch attached.

    @amauryfa
    Copy link
    Member

    Which problem are you trying to solve?
    Can this change be tested somehow?

    @pitrou
    Copy link
    Member

    pitrou commented Mar 14, 2012

    Are you sure this is desired? Nowhere can I think of a place in the stdlib where we use overlapped I/O on sockets.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Mar 14, 2012

    pitrou wrote:

    Are you sure this is desired? Nowhere can I think of a place in the
    stdlib where we use overlapped I/O on sockets.

    multiprocessing.connection.wait() does overlapped zero length reads on sockets. It's documentation currently claims that it works with sockets.

    Also it would seem strange if some sockets (created with socket()) have the overlapped attribute, but some others (created with WSASocket()) don't.

    amaury.forgeotdarc wrote:

    Which problem are you trying to solve?

    For one thing, the fact that socketmodule.c does not obey the word "must" in the quote from Microsoft's documentation.

    Can this change be tested somehow?

    An additional test could be added to test_multiprocessing.TestWait.

    Slightly surprisingly, in the testing I have done so far, using wait() with a duplicated socket seems to work without the patch. However, I would be rather wary of just assuming that it works in all cases and on all versions of Windows.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2012

    New changeset 5be4d8fc9c44 by Antoine Pitrou in branch 'default':
    Issue bpo-14300: Under Windows, sockets created using socket.dup() now allow overlapped I/O.
    http://hg.python.org/cpython/rev/5be4d8fc9c44

    @pitrou
    Copy link
    Member

    pitrou commented Mar 31, 2012

    Ok, I've committed the patch to 3.3 since it can be useful with the new wait() method. Thanks!

    @pitrou pitrou closed this as completed Mar 31, 2012
    @kristjanvalur
    Copy link
    Mannequin

    kristjanvalur mannequin commented Apr 1, 2012

    I already included this fix in my "socket share" patch, see bpo-14310.

    I think this was a bug that should be checked in to all relevant branches. The reason is this text from msdn documentation for WsaDuplicateSocket:
    " Both the source process and the destination process should pass the same flags to their respective WSASocket function calls. If the source process uses the socket function to create the socket, the destination process _must_ [underline KVJ] pass the WSA_FLAG_OVERLAPPED flag to its WSASocket function call."

    See http://msdn.microsoft.com/en-us/library/windows/desktop/ms741565(v=vs.85).aspx

    @kristjanvalur
    Copy link
    Mannequin

    kristjanvalur mannequin commented Apr 1, 2012

    Also, see this: http://support.microsoft.com/kb/179942/EN-US
    applies to windows 2000 only, as far as I can tell, though. Don't know if we still support that.

    I have scoured the docs, but found yet no reason to _not_ use this attribute. I wonder why it is optional at all.

    @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
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants