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

Test ConPTY backend against pywinpty 2.0.5 #146

Merged
merged 2 commits into from
Mar 7, 2022

Conversation

andfoy
Copy link
Member

@andfoy andfoy commented Mar 4, 2022

No description provided.

@andfoy
Copy link
Member Author

andfoy commented Mar 4, 2022

@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
Copy link
Contributor

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
Copy link
Member Author

andfoy commented Mar 4, 2022

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
Copy link
Member Author

andfoy commented Mar 4, 2022

Got it:
imagen

@andfoy
Copy link
Member Author

andfoy commented Mar 4, 2022

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 andfoy force-pushed the conpty_winpty_2.0.4 branch from d841366 to 47ce418 Compare March 5, 2022 04:50
@andfoy
Copy link
Member Author

andfoy commented Mar 5, 2022

@blink1073 this is now working as expected

@blink1073 blink1073 added the bug label Mar 5, 2022
@blink1073
Copy link
Contributor

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
Copy link
Contributor

(so remove lines 49 and 50 altogether)

@andfoy
Copy link
Member Author

andfoy commented Mar 7, 2022

@blink1073, done

@andfoy andfoy changed the title Test ConPTY backend against pywinpty 2.0.4 Test ConPTY backend against pywinpty 2.0.5 Mar 7, 2022
@blink1073
Copy link
Contributor

Cool, thanks!

@blink1073 blink1073 merged commit 2cccad6 into jupyter:main Mar 7, 2022
@andfoy andfoy deleted the conpty_winpty_2.0.4 branch March 7, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants