-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(tests): add end-to-end tests for shell PTY and session management #1424
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
Changes from all commits
71a979e
511f2d8
e0a3b15
bd7b171
21afb60
7778629
f03cb65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -118,6 +118,9 @@ async def _read_stream(stream: AsyncReadable, cb: Callable[[bytes], None]): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return await process.wait() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except asyncio.CancelledError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await process.kill() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When cancellation lands right after the subprocess has already exited, Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except TimeoutError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await process.kill() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+122
to
126
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await process.kill() | |
| raise | |
| except TimeoutError: | |
| await process.kill() | |
| raise | |
| try: | |
| await process.kill() | |
| try: | |
| # Ensure the process is reaped to avoid zombies. | |
| await asyncio.wait_for(process.wait(), 1.0) | |
| except Exception: | |
| # Ignore errors during best-effort cleanup. | |
| pass | |
| finally: | |
| raise | |
| except TimeoutError: | |
| try: | |
| await process.kill() | |
| try: | |
| # Ensure the process is reaped to avoid zombies. | |
| await asyncio.wait_for(process.wait(), 1.0) | |
| except Exception: | |
| # Ignore errors during best-effort cleanup. | |
| pass | |
| finally: | |
| raise |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If cancellation is delivered just after the subprocess has already finished (for example, user presses Esc as a short command exits), this unconditional
kill()can raiseProcessLookupError(asasyncio.subprocess.Process.kill()does on exited processes). That replaces the expectedCancelledErrorwith an unexpected failure, so the shell turn may surface an internal error instead of a clean interruption; checkprocess.returncodeor suppressProcessLookupErrorduring cleanup.Useful? React with 👍 / 👎.