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.

Sign up for GitHub

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

Jump to bottom

PR: Add test that terminate causes EOFError #213

Merged
merged 2 commits into from
Mar 2, 2022
Merged

PR: Add test that terminate causes EOFError #213

merged 2 commits into from
Mar 2, 2022

Conversation

blink1073
Copy link
Collaborator

@blink1073 blink1073 commented Feb 17, 2022 •
edited
Loading

@blink1073
Copy link
Collaborator Author

Okay, this caught a bug where we were using SIGKILL, but it doesn't yet explain what I'm seeing in terminado. Will leave this open while I'm still debugging.

@blink1073
Copy link
Collaborator Author

Okay, I think what happened is f105bcc removed the ability to detect EOF passively using a select listening on PtyProcess.fd. When we manually read as I've done in the tests, we get the EOF condition. I need to make a test that uses a select loop that fails, and then fix the behavior.

@blink1073
Copy link
Collaborator Author

Note to self: we need to use loop.add_reader with asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) to simulate how it is handled by terminado.

@blink1073
Copy link
Collaborator Author

blink1073 commented Feb 24, 2022 •
edited
Loading

I think ConPTY doesn't like reading after EOF. Next thing I'll try is disabling that read, but only when using the ConPTY backend, since it might result in lost data.

@blink1073
Copy link
Collaborator Author

@andfoy I think pytest 7 made the tests in master start failing, I kicked the most recent job and it is also timing out with ConPTY and test_intr(). I'm trying to debug now.

@andfoy
Copy link
Owner

andfoy commented Mar 1, 2022

Thanks @blink1073 for tracking this, what happens if pytest is ran with -s? Stream redirection is difficult to track on Windows

@blink1073
Copy link
Collaborator Author

I tried that, and running pytest directly. I'll try pinning pytest next.

@blink1073
Copy link
Collaborator Author

Well, it isn't pytest...

@andfoy
Copy link
Owner

andfoy commented Mar 1, 2022

Maybe getting access into any CI worker via RDP may give us more information: https://github.com/spyder-ide/pywinpty/blob/b499f20711879bee8b0edcf00919fc78f09e51bc/.github/workflows/windows_build.yml#L55

You need an NGROK account to update this token https://github.com/spyder-ide/pywinpty/blob/b499f20711879bee8b0edcf00919fc78f09e51bc/.github/workflows/windows_build.yml#L64

@blink1073
Copy link
Collaborator Author

I don't have the bandwidth to debug ConPTY, my only aim is to get terminado working with WinPTY again.

@andfoy
Copy link
Owner

andfoy commented Mar 1, 2022

You could skip the tests for conpty entirely and see if it passes

@blink1073
Copy link
Collaborator Author

Yep, I did that and all tests pass with WinPTY.

@andfoy
Copy link
Owner

andfoy commented Mar 1, 2022

You could set PYWINPTY_BACKEND=1

@blink1073
Copy link
Collaborator Author

My vote would be to skip the specific stalling ConPTY tests, cut a release that fixes terminado, and leave an open issue for full ConPTY support.

@blink1073 blink1073 marked this pull request as ready for review March 2, 2022 19:46
@blink1073
Copy link
Collaborator Author

Okay, ready for review. Once this is merged I'll kick the dependabot PRs, and open an issue tracking the ConPTY regression.

@ccordoba12
Copy link
Contributor

@blink1073, could you squash your testing commits and leave the ones that actually implemented the changes necessary for terminado? Thanks!

blink1073 added 2 commits March 2, 2022 15:52
@blink1073
test that terminate causes EOFError
5770274 
try with sigterm

try with sigkill too

update tests

add loop test

add explicit EOF

another attempt

cleanup

try again

don't skip the test

skip control tests for now

use new event loop

reinstate control tests

fix handling of event loop

follow docs

try skipping the loop test

try conpty only

remove more winpty

see if removing the extra read works

try without extra write and no loop test

back to original code

try winpty only

timeout after 15 minutes

try with conpty only

remove eof handling

try without capture

run pytest directly

pin pytest

fix shell quoting

use 10 minute timeout
@blink1073
xfail on some tests with conpty
336fe01 
fix backend parameter

add another skip

add another skip

revert changes to test running

remove pytest pin
@blink1073
Copy link
Collaborator Author

could you squash your testing commits and leave the ones that actually implemented the changes necessary for terminado

Done!

@andfoy andfoy changed the title Test that terminate causes EOFError PR: Add test that terminate causes EOFError Mar 2, 2022
@andfoy andfoy added this to the v2.0.3 milestone Mar 2, 2022
@andfoy
Copy link
Owner

andfoy commented Mar 2, 2022

Thanks @blink1073, I'll draft a new release once I merge the dependency PRs

@andfoy andfoy merged commit 5abb786 into andfoy:master Mar 2, 2022
@blink1073 blink1073 deleted the check-eof branch March 2, 2022 22:14
@ccordoba12
Copy link
Contributor

Done!

Thanks!

And also thank you for your hard work on solving this problem!

@blink1073 blink1073 mentioned this pull request Mar 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andfoy andfoy andfoy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@blink1073 @andfoy @ccordoba12