Add support for Windows platform #26
Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
|
Hi @yuvipanda and @manics, Can you review this PR? |
| @@ -16,6 +16,10 @@ | |||
| (signal.SIGINT, 5), | |||
| ], | |||
| ) | |||
| @pytest.mark.skipif( | |||
| sys.platform == "win32", | |||
| reason="Testing signals on Windows doesn't seem to be possible", | |||
There was a problem hiding this comment.
What happens if you try to test this on Windows?
There was a problem hiding this comment.
The tests either fail or hang on Windows.
From what I have seen, on Windows, child processes of this test framework are unable to decode the signals being sent to them resulting in unstable behavior. eg: test_asyncatexit fails as opposed to test_signals which hangs.
This seems to be an artifact of the testing infrastructure, as our application that uses jupyter-server-proxy is able to accept and handle signals correctly on Windows.
|
@yuvipanda what are your thoughts on the updates that @rashedmyt has made to the repository? Do they match your vision? |
|
@prabhakk-mw @rashedmyt thank you for working through with me on this! I made one suggested change but otherwise this looks great to me. Much clearer than having this sprinkled throughout! I'm going to give a few days for others to chime in if they wish, but am happy to merge once the suggested change is discussed / accepted. |
Co-authored-by: Yuvi Panda <yuvipanda@gmail.com>
There was a problem hiding this comment.
This looks great!!
I really appreciate seeing the UNIX/Windows specific changes encapsulated in the choice of a class, it made this feature very easy to think about and probably easier to maintain going onwards! Thanks @rashedmyt and @yuvipanda for iterating towards that!
Thank you @rashedmyt for your thorough work!!! ❤️ 🎉 🌻
|
Congrats on your first merged pull request in this project! 🎉 |
Summary
This PR enables simpervisor to manage processes on the Windows Operating System. This in turn enables jupyter-server-proxy to proxy processes on Windows as well.
Description
Windows support for jupyter-server-proxy is not available as of today. The root cause of the issue is the API (asyncio.create_subprocess_exec) used for creating child processes. Jupyter uses tornado package for its entire web infrastructure and tornado uses asyncio everywhere. asyncio has SelectorEventLoop on POSIX and ProactorEventLoop on Windows as the default. However, tornado overrides this default behavior and uses SelectorEventLoop on both POSIX and Windows. The issue arises because jupyter-server-proxy uses the global event loop, which indirectly uses SelectorEventLoop on Windows, which does not support creating child process.
jupyterhub/jupyter-server-proxy#181 has attempted to fix this issue by explicitly using ProactorEventLoop on Windows platform. However, ProactorEventLoop does not support signal handlers and hence some of the functionality is missing.
This PR aims to fix the issue by replacing the existing APIs of asyncio package with APIs from subprocess and signal package. The proposed implementation aims to retain most if not all of the existing features.
Fixes #6 , jupyterhub/jupyter-server-proxy#147
List of changes
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY.