search

jupyter / terminado

Terminals served by tornado websockets
http://terminado.readthedocs.org/en/latest/
BSD 2-Clause "Simplified" License
368 stars 95 forks source link

Test ConPTY backend against pywinpty 2.0.5 #146

Closed andfoy closed 2 years ago

andfoy commented 2 years ago

@blink1073, this is mostly a concurrency error, the time it takes to kill the process is larger than the time that it takes to set the closed attribute. In this case, we would like to have a thread-safe behaviour here. So we either wait a little bit before asserting that the process is down, or we make the access to the closed attribute atomic in pywinpty

blink1073 commented 2 years ago

I think it is fair to add an asyncio.sleep loop in the tests here. We could check right away and then once a second for 5 seconds.

andfoy commented 2 years ago

I thought that the terminate method of ptyprocess was being called, but I saw there is a loop in management that calls isalive. Then there's some kind of strange result being returned by GetExitCodeProcess, I'll proceed to try to reproduce this locally

andfoy commented 2 years ago

Got it: imagen

andfoy commented 2 years ago

According to https://stackoverflow.com/a/41464804/1143629, we shouldn't be polling with GetExitCodeProcess to see if a process is alive. I'll need to go back to winpty-rs

andfoy commented 2 years ago

@blink1073 this is now working as expected

blink1073 commented 2 years ago

I'm thinking we should rely on the env variable to set the backend, since there isn't yet a way to configure this setting using traits.

blink1073 commented 2 years ago

(so remove lines 49 and 50 altogether)

andfoy commented 2 years ago

@blink1073, done

blink1073 commented 2 years ago

Cool, thanks!