Use selectors instead of select.poll in sync.WebSocket Server for multi-platform support #1349
Conversation
|
Thank you for doing the research. I considered using the selectors module when I wrote that code but wasn't quite sure about the benefits of the additional abstraction layer. |
|
With some additional testing today, I noticed that Mac OS X doesn't return from the |
This is almost certainly why I bother with the |
On the Win32 platform, only sockets can be used with I/O multiplexing (such as that performed by selectors.DefaultSelector); the pipe cannot be added to the selector. However, on the win32 platform, simply closing the listener socket is enough to cause the call to select to return -- the additional pipe is redundant. On Mac OS X (and possibly other BSD derivatives), closing the listener socket isn't enough. In the interest of maximum compatibility, we simply disable the use of os.pipe on the Win32 platform.
|
I pushed an additional commit to restore the use of os.pipe unless the runtime platform is win32. |
|
I see that the test coverage is now failing, but I'm uncertain how to address it. If you can give me a little guidance, I'll be happy to put in some time to see if I can fix it. |
|
Let's add For the purposes of coverage testing, we cannot realistically get branch coverage here with a CI that runs on Linux and this isn't a sufficient reason to run coverage testing on Windows. This should work without any changes to the test suite.. |
|
As far as I understand, this issue makes the new sync API for servers unusable under Windows. I think this deserves a bugfix release. I can take care of backporting to the 11.x branch and making a 11.0.3 release. |
|
Don't bother with the changelog; I'll take care of it when I do the backport. |
This was indeed my motivation for fixing it. |
|
Do you need anything else from me on this PR? |
|
Thank you, I don't need anything else. I was on holidays last week. Probably I'll do it today or tomorrow. |
…ti-platform support (#1349) * use multiplatform selector instead of poll * don't use os.pipe with the I/O multiplexing selector on win32 On the Win32 platform, only sockets can be used with I/O multiplexing (such as that performed by selectors.DefaultSelector); the pipe cannot be added to the selector. However, on the win32 platform, simply closing the listener socket is enough to cause the call to select to return -- the additional pipe is redundant. On Mac OS X (and possibly other BSD derivatives), closing the listener socket isn't enough. In the interest of maximum compatibility, we simply disable the use of os.pipe on the Win32 platform. * exclude platform checks for win32 from coverage testing
|
Version 11.0.3 will be available on PyPI as soon as https://github.com/python-websockets/websockets/actions/runs/4907582069 completes. |
|
Thank you! |
For a senior-level university network apps design course, my students were using the threaded version of
WebSocketServeras part of their final project. While the project required their work to run successfully in containers on Linux, many of them use Windows for their development workstation. We discovered thatserve_foreverthrows an error at startup on Windows due to the use ofselect.pollas the means to block while waiting for incoming socket connections. The documentation forpollindicates that it is not supported on all platforms, and apparently Windows is one such platform.I patched
WebSocketServerto instead useselectors.DefaultSelectorwhich determines the best supported mechanism for I/O multiplexing on the runtime platform. The functionality is the same, but it works on a wider range of platforms.After switching to
selectors.DefaultSelector, I discovered that Windows also doesn't support I/O multiplexing using pipes or files -- only sockets. I removed the use ofos.pipefor the shutdown mechanism. It was redundant anyway -- simply closing the listener socket in theshutdownmethod is sufficient to cause the selector to return. Subsequently, the call tosocket.accept(on the closed listener socket) causes the loop inserve_foreverto terminate as expected.I tested the change on Windows, Mac OS X, and a Linux container, and it seems that it correctly supports all three platforms.