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 loop.sock_sendall() fails on Windows when sockets are shared across threads #122240

Closed
NoahStapp opened this issue Jul 24, 2024 · 17 comments
Labels
stdlib Python modules in the Lib dir topic-asyncio type-feature A feature request or enhancement

Comments

@NoahStapp
Copy link

NoahStapp commented Jul 24, 2024

Bug report

Bug description:

asyncio's loop.sock_sendall() method causes a Windows OSError: [WinError 87] The parameter is incorrect error when using a socket that was created in a different thread. This error only appears when using the default ProactorEventLoop on Windows.

Minimum reproducible example:

import asyncio
import threading
import socket

socks = []


async def create_socket():
    s = socket.socket()
    s.connect(("www.python.org", 80))
    s.settimeout(0.0)
    loop = asyncio.get_event_loop()
    print(f"{threading.current_thread().name}: {await asyncio.wait_for(loop.sock_sendall(s, bytes('hello', 'utf-8')), timeout=5)}")

    socks.append(s)


async def use_socket():
    while len(socks) < 1:
        pass
    s = socks.pop()
    loop = asyncio.get_event_loop()
    print(f"{threading.current_thread().name}: {await asyncio.wait_for(loop.sock_sendall(s, bytes('hello', 'utf-8')), timeout=5)}")


def wrapper(func):
    asyncio.run(func())


t1 = threading.Thread(target=wrapper, args=(create_socket,))
t2 = threading.Thread(target=wrapper, args=(use_socket,))

t1.start()
t2.start()

Error stacktrace:

  File "C:\Python312\Lib\threading.py", line 1073, in _bootstrap_inner
    self.run()
  File "C:\Python312\Lib\threading.py", line 1010, in run
    self._target(*self._args, **self._kwargs)
  File "C:\cygwin\home\Administrator\mongo-python-driver\windows_test.py", line 27, in wrapper
    asyncio.run(func())
  File "C:\Python312\Lib\asyncio\runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "C:\Python312\Lib\asyncio\runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python312\Lib\asyncio\base_events.py", line 687, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "C:\cygwin\home\Administrator\mongo-python-driver\windows_test.py", line 23, in use_socket
    print(f"{threading.current_thread().name}: {await asyncio.wait_for(loop.sock_sendall(s, bytes('hello
', 'utf-8')), timeout=5)}")
                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python312\Lib\asyncio\tasks.py", line 520, in wait_for
    return await fut
           ^^^^^^^^^
  File "C:\Python312\Lib\asyncio\proactor_events.py", line 721, in sock_sendall
    return await self._proactor.send(sock, data)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python312\Lib\asyncio\windows_events.py", line 539, in send
    self._register_with_iocp(conn)
  File "C:\Python312\Lib\asyncio\windows_events.py", line 709, in _register_with_iocp
    _overlapped.CreateIoCompletionPort(obj.fileno(), self._iocp, 0, 0)
OSError: [WinError 87] The parameter is incorrect

CPython versions tested on:

3.8, 3.11, 3.12

Operating systems tested on:

Linux, macOS, Windows

@NoahStapp NoahStapp added the type-bug An unexpected behavior, bug, or error label Jul 24, 2024
@graingert
Copy link
Contributor

fyi this is reproducible when running on the same thread:

import asyncio
import threading
import socket

socks = []


async def create_socket():
    s = socket.socket()
    s.connect(("www.python.org", 80))
    s.settimeout(0.0)
    loop = asyncio.get_event_loop()
    print(f"{threading.current_thread().name}: {await asyncio.wait_for(loop.sock_sendall(s, bytes('hello', 'utf-8')), timeout=5)}")

    socks.append(s)


async def use_socket():
    s = socks.pop()
    loop = asyncio.get_event_loop()
    print(f"{threading.current_thread().name}: {await asyncio.wait_for(loop.sock_sendall(s, bytes('hello', 'utf-8')), timeout=5)}")

asyncio.run(create_socket())
asyncio.run(use_socket())

@graingert
Copy link
Contributor

a smaller reproducer, that doesn't depend on an external server:

import asyncio
import sys
import socket

async def sock_sendall(sock, data):
    return await asyncio.get_running_loop().sock_sendall(sock, data)

def main():
    s1, s2 = socket.socketpair()
    with s1, s2:
        s1.setblocking(False)
        asyncio.run(sock_sendall(s1, b"\x00"))
        asyncio.run(sock_sendall(s1, b"\x00"))

if __name__ == "__main__":
    sys.exit(main())

@ShaneHarvey
Copy link
Contributor

It looks like the problem is that when a ProactorEventLoop first encounters a new file it will cache it and call CreateIoCompletionPort:

        # To get notifications of finished ops on this objects sent to the
        # completion port, were must register the handle.
        if obj not in self._registered:
            self._registered.add(obj)
            _overlapped.CreateIoCompletionPort(obj.fileno(), self._iocp, 0, 0)

https://github.com/python/cpython/blob/v3.13.0b4/Lib/asyncio/windows_events.py#L706-L710

Once registered, Windows will not allow CreateIoCompletionPort to be called again with another completion port. For example, here's an even smaller example which repros the The parameter is incorrect error:

import _overlapped
import _winapi
import sys
import socket


def main():
    s1, s2 = socket.socketpair()
    with s1, s2:
        s1.setblocking(False)
        iocp1 = _overlapped.CreateIoCompletionPort(_overlapped.INVALID_HANDLE_VALUE, _winapi.NULL, 0, _winapi.INFINITE)
        _overlapped.CreateIoCompletionPort(s1.fileno(), iocp1, 0, 0)
        iocp2 = _overlapped.CreateIoCompletionPort(_overlapped.INVALID_HANDLE_VALUE, _winapi.NULL, 0, _winapi.INFINITE)
        _overlapped.CreateIoCompletionPort(s1.fileno(), iocp2, 0, 0)


if __name__ == "__main__":
    sys.exit(main())

The only way to fix this would be to clear the Windows (and ProactorEventLoop) state. Starting in Windows 8.1 it's possible to remove a completion port from a file via FileReplaceCompletionInformation:

import _overlapped
import _winapi
import sys
import socket
import ctypes
from ctypes.wintypes import HANDLE

# See: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntsetinformationfile
FileReplaceCompletionInformation = 61

# See: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_completion_information
class FileCompletionInformation(ctypes.Structure):
    _fields_ = [
        ("Port", HANDLE),
        ("Key", ctypes.c_void_p),
    ]

# See: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/ns-wdm-_io_status_block
class PointerOrStatus(ctypes.Union):
    _fields_ = [("Status", ctypes.c_long),
                ("Pointer", ctypes.c_void_p)]

class IoStatusBlock(ctypes.Structure):
    _anonymous_ = ("u",)
    _fields_ = [
        ("u", PointerOrStatus),
        ("Information", ctypes.c_void_p),
    ]

def clear_iocp(file):
    # Clear association, FileReplaceCompletionInformation was added in Windows 8.1:
    # __kernel_entry NTSYSCALLAPI NTSTATUS NtSetInformationFile(
    #   [in]  HANDLE                 FileHandle,
    #   [out] PIO_STATUS_BLOCK       IoStatusBlock,
    #   [in]  PVOID                  FileInformation,
    #   [in]  ULONG                  Length,
    #   [in]  FILE_INFORMATION_CLASS FileInformationClass
    # );
    file_info = FileCompletionInformation(None, None)
    out = IoStatusBlock()
    ctypes.windll.ntdll.NtSetInformationFile(file.fileno(), ctypes.byref(out), file_info, ctypes.sizeof(file_info), FileReplaceCompletionInformation)


def main():
    s1, s2 = socket.socketpair()
    with s1, s2:
        s1.setblocking(False)
        iocp1 = _overlapped.CreateIoCompletionPort(_overlapped.INVALID_HANDLE_VALUE, _winapi.NULL, 0, _winapi.INFINITE)
        _overlapped.CreateIoCompletionPort(s1.fileno(), iocp1, 0, 0)
        clear_iocp(s1)
        iocp2 = _overlapped.CreateIoCompletionPort(_overlapped.INVALID_HANDLE_VALUE, _winapi.NULL, 0, _winapi.INFINITE)
        _overlapped.CreateIoCompletionPort(s1.fileno(), iocp2, 0, 0)


if __name__ == "__main__":
    sys.exit(main())

How to integrate this into the asyncio API is an interesting problem.

@ShaneHarvey
Copy link
Contributor

ShaneHarvey commented Jul 25, 2024

Putting it all together, this works around the error:

def clear_iocp(file):
    # clear_iocp et al. from above 

async def sock_sendall(sock, data):
    return await asyncio.get_running_loop().sock_sendall(sock, data)

def main():
    s1, s2 = socket.socketpair()
    with s1, s2:
        s1.setblocking(False)
        loop = asyncio.ProactorEventLoop()
        asyncio.set_event_loop(loop)
        asyncio.run(sock_sendall(s1, b"\x00"))

        # Discard state:
        loop._proactor._registered.discard(s1)
        clear_iocp(s1)

        asyncio.run(sock_sendall(s1, b"\x00"))


if __name__ == "__main__":
    sys.exit(main())

@kumaraditya303
Copy link
Contributor

This feels like misuse of API to me, you should not be using two asyncio.run to create socket and using it in different event loops.

@kumaraditya303 kumaraditya303 added the pending The issue will be closed if no feedback is provided label Oct 29, 2024
@graingert
Copy link
Contributor

This feels like misuse of API to me, you should not be using two asyncio.run to create socket and using it in different event loops.

It should absolutely be possible for asyncio to adopt a passed in socket and later relinquish it for other usage

@kumaraditya303
Copy link
Contributor

It should absolutely be possible for asyncio to adopt a passed in socket and later relinquish it for other usage

That sounds like a new feature than a bug to me

@kumaraditya303 kumaraditya303 added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error pending The issue will be closed if no feedback is provided labels Nov 2, 2024
@asvetlov
Copy link
Contributor

asvetlov commented Nov 2, 2024

Well, if #122240 (comment) snipped fixes the problem (sorry, I have no access to Windows box now to check it) the fix could be easy.

IocpProactor.close() could clean-up self._registered weakset. I see no downsides for this approach, the cleanup shouldn't break any existing code.

A pull request with fix and tests is welcome!

@asvetlov
Copy link
Contributor

asvetlov commented Nov 2, 2024

In practical sense the distinguish between new feature and bugfix is an answer on the following question: should we backport the fix or put it into upcoming Python release only.

I have no strong opinion on this, the issue affects a few users I guess.

@kumaraditya303
Copy link
Contributor

Yeah, my understanding is that this has never worked so it would be better to put it in a new release rather than backports hence I added feature label

@Kludex
Copy link

Kludex commented Dec 15, 2024

It would be cool to have this working. It would allow me to remove this logic in Uvicorn:
https://github.com/encode/uvicorn/blob/7983c1ae9c2276b94cd85217f7aa58bb248847c4/uvicorn/loops/asyncio.py#L9-L10

@graingert
Copy link
Contributor

@Kludex I wonder if you can avoid it by os.dup-ing the socket?

@kumaraditya303
Copy link
Contributor

I think we have avoided in the past to use the NT APIs, but I am not an Windows expert.

@zooba Do you have any opinion on the fix #122240 (comment)?

@graingert
Copy link
Contributor

graingert commented Dec 21, 2024

turns out you can't avoid the isssue by .dup()-ing the socket

one odd thing is that putting the socket in this IOCP registered state doesn't seem to phase trio, which also uses the proactor on windows.

import trio.socket
import asyncio
import sys
import socket

async def sock_sendall(sock, data):
    return await asyncio.get_running_loop().sock_sendall(sock, data)

async def trio_socksendall(sock, data):
    await trio.SocketStream(trio.socket.from_stdlib_socket(sock)).send_all(data)
    return len(data)

def main():
    s1, s2 = socket.socketpair()
    with s1, s2:
        s1.setblocking(False)
        try:
            asyncio.run(sock_sendall(s1, b"\x00"))  # 1
            trio.run(trio_socksendall, s1, b"\x00")  # 2
            asyncio.run(sock_sendall(s1, b"\x00"))  # 3
        finally:
            print(s2.recv(1024))

if __name__ == "__main__":
    sys.exit(main())

as you can see trio still manages to write the byte to the socket:

PS C:\Users\User\demo_project> & c:/Users/User/demo_project/.venv/Scripts/python.exe c:/Users/User/demo_project/demo_socket.py
b'\x00\x00'
Traceback (most recent call last):
  File "c:\Users\User\demo_project\demo_socket.py", line 25, in <module>
    sys.exit(main())
             ^^^^^^
  File "c:\Users\User\demo_project\demo_socket.py", line 20, in main
    asyncio.run(sock_sendall(s1, b"\x00"))  # 3
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.1264.0_x64__qbz5n2kfra8p0\Lib\asyncio\runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.1264.0_x64__qbz5n2kfra8p0\Lib\asyncio\runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.1264.0_x64__qbz5n2kfra8p0\Lib\asyncio\base_events.py", line 687, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "c:\Users\User\demo_project\demo_socket.py", line 7, in sock_sendall
    return await asyncio.get_running_loop().sock_sendall(sock, data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.1264.0_x64__qbz5n2kfra8p0\Lib\asyncio\proactor_events.py", line 721, in sock_sendall
    return await self._proactor.send(sock, data)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.1264.0_x64__qbz5n2kfra8p0\Lib\asyncio\windows_events.py", line 539, in send
    self._register_with_iocp(conn)
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.1264.0_x64__qbz5n2kfra8p0\Lib\asyncio\windows_events.py", line 709, in _register_with_iocp
    _overlapped.CreateIoCompletionPort(obj.fileno(), self._iocp, 0, 0)
OSError: [WinError 87] The parameter is incorrect

I think because it does fancy tricks to enable wait_readable and wait_writable via IOCP

@zooba
Copy link
Member

zooba commented Dec 28, 2024

I think we have avoided in the past to use the NT APIs, but I am not an Windows expert.

Greatly prefer not to, and my intent is that they're only used dynamically with a working fallback and as much verification as we can possibly perform. In other words, for significant optimisations, not for base correctness (if the OS doesn't intend you to do it, then CPython doesn't support it).

It should absolutely be possible for asyncio to adopt a passed in socket and later relinquish it for other usage

This sounds great, but potentially impossible.

It also sounds like an admission that asyncio is not suitable for building an application, if it is necessary to "leave" it. That concerns me, and makes me more inclined to discourage doing unusual things (such as private OS APIs).

What's the legitimate scenario that we want to encourage through this feature? Alternatively, what else do we need to fix so that developers don't need to do this anymore?

@picnixz picnixz added the stdlib Python modules in the Lib dir label Dec 28, 2024
@kumaraditya303
Copy link
Contributor

I'll close this as wont-fix, the use-cases are unclear to me and the OS itself doesn't support it without messing with internals.

@kumaraditya303 kumaraditya303 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 20, 2025
@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Jan 20, 2025
@ShaneHarvey
Copy link
Contributor

The use case here is when you want to share a resource like a connection pool across threads or across event loops. Currently this is possible on Linux and macOS but not possible on Windows due to this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir topic-asyncio type-feature A feature request or enhancement
Projects
Status: Done
Development

No branches or pull requests

8 participants