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

TrioEventLoop doesn't support subprocess_exec #97

Open
ZeeD opened this issue Jan 5, 2021 · 9 comments
Open

TrioEventLoop doesn't support subprocess_exec #97

ZeeD opened this issue Jan 5, 2021 · 9 comments

Comments

@ZeeD
Copy link

ZeeD commented Jan 5, 2021

I'm having NotImplementedError trying to run an asyncio-based library (chess) that run an external process with trio-asyncio.
It seems that subprocess_exec is not working as intended.
Minimal example that shows the error:

import trio
import trio_asyncio
import asyncio


async def main() -> None:
    async with trio_asyncio.open_loop() as loop:
        running_loop = asyncio.events.get_running_loop()
        assert loop is running_loop

        await loop.subprocess_exec(asyncio.SubprocessProtocol, 'dir')

trio.run(main)

the error:

Traceback (most recent call last):
  File "C:\Users\vito.detullio\Desktop\workspace-mystuff\moves\_dbg\minimal.py", line 13, in <module>
    trio.run(main)
  File "C:\Users\vito.detullio\Desktop\workspace-mystuff\moves_VENV\lib\site-packages\trio\_core\_run.py", line 1928, in run
    raise runner.main_task_outcome.error
  File "C:\Users\vito.detullio\Desktop\workspace-mystuff\moves\_dbg\minimal.py", line 11, in main
    await running_loop.subprocess_exec(asyncio.SubprocessProtocol, 'dir')
  File "C:\Program Files\Python38\lib\asyncio\base_events.py", line 1615, in subprocess_exec
    transport = await self._make_subprocess_transport(
  File "C:\Program Files\Python38\lib\asyncio\base_events.py", line 487, in _make_subprocess_transport
    raise NotImplementedError
NotImplementedError
@pquentin
Copy link
Member

pquentin commented Jan 5, 2021

Sorry that you're having this issue. The root cause is possibly #85 which is also a Windows-specific issue. Would you like to take a stab at it?

@pquentin
Copy link
Member

pquentin commented Jan 5, 2021

To know if this is the same issue, can you try adding raise NotImplementedError() at the beginning of add_signal_handler in trio_asyncio_base.py and try again? That file is probably in C:\Users\vito.detullio\Desktop\workspace-mystuff\moves_VENV\lib\site-packages\trio_asyncio.

@ZeeD
Copy link
Author

ZeeD commented Jan 5, 2021

Hi. Thanks for you response.
I cannot find any trio_asyncio_base.py in the trio_asyncio package. There is a add_signal_handler method in _base.py

zed@FGVIL021718:/mnt/c/Users/vito.detullio/Desktop/workspace-mystuff/moves_VENV/Lib/site-packages/trio_asyncio$ grep -r 'add_signal_handler' .
./_base.py:    def add_signal_handler(self, sig, callback, *args):
Binary file ./__pycache__/_base.cpython-38.pyc matches
$

I changed it following your suggestion but there is no difference in behavior

@ZeeD
Copy link
Author

ZeeD commented Jan 5, 2021

Moreover if I try with a pure asyncio approach I see that the even loop is an instance of asyncio.windows_events.ProactorEventLoop that simply implements the method _make_subprocess_transport, that is missing in TrioEventLoop. I'm not sure that #85 is related here. Or did some specific subclass should be present instead of TrioEventLoop ?

@ZeeD
Copy link
Author

ZeeD commented Jan 5, 2021

I had a quick look at the code and found simply that windows is "skipped":

if not _mswindows:
async def _make_subprocess_transport(
self, protocol, args, shell, stdin, stdout, stderr, bufsize, extra=None, **kwargs
):
"""Make a subprocess transport. Asyncio context."""

(there is no else)

@njsmith
Copy link
Member

njsmith commented Jan 5, 2021

Yeah, IIRC trio-asyncio's subprocess support is just broken on Windows. On Unix it mostly re-uses asyncio's native subprocess support with some hacks to make it play nicely with trio, which works OK because on Unix subprocesses mostly just need wait_readable and wait_writable. But on Windows subprocesses are implemented in a completely different way, and we can't really re-use asyncio's code. I think it was done this way because (a) it was easier than reimplementing asyncio's subprocess API from scratch, (b) at the time this code was written, trio didn't even have native subprocess support.

Probably the ideal solution would be to reimplement asyncio's subprocess API on top of trio's native subprocess API (which does exist now). It's more work because we have to implement our own subprocess transport etc., but OTOH it would mean that it works consistently across all platforms, and automatically take advantage of improvements in trio (e.g. using pidfd support when available).

@smurfix
Copy link
Collaborator

smurfix commented Jan 5, 2021

Another reason for the problem is that, while you did open a trio_asyncio loop, you're still in Trio context when you call await loop.subprocess_exec(). You need to wrap that in aio_as_trio, and I need to add a sentence or two to that effect to the documentation.

@ZeeD
Copy link
Author

ZeeD commented Jan 5, 2021

@smurfix sorry, I tough that was "transparent" for the called code... I was looking at the examples on https://trio-asyncio.readthedocs.io/en/latest/usage.html#trio-main-loop

@smurfix
Copy link
Collaborator

smurfix commented Jan 5, 2021

Oops, that's an error. Will fix.

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

4 participants