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

Fix issue where large stdin writes can cause Tornado to hang #189

Closed KoopaKing closed 1 year ago

KoopaKing commented 1 year ago

A fix for #183

This includes switching to using a dedicated executor for blocking IO, specifically wrapping calls to ptyprocess.write which, can hang when the PTY buffer (the size of which is set by the OS) is exceeded. TermSocket users can provide their own thread pool by using the named blocking_io_executor parameter on initialization, but by default a shared, process-wide single threaded executor will be used.

This also includes a regression test which will fail on Debian and OSX (but may be able to pass on some OSs due to the variance of the PTY buffer's size).

This also switched the default filetype of terminado/tests/basic_test.py from dos to unix to match with the rest of the files in this package and prevent some editors from auto-inserting carriage returns on save.

KoopaKing commented 1 year ago

Ah, seems like my assertion about the output size doesn't work on windows. Probably windows doesn't have echo, idk, I'm not a windower. I'll update this PR in a bit to fix this.

blink1073 commented 1 year ago

I'd say skip that test on Windows.

KoopaKing commented 1 year ago

Yeah, I'm doing that real quick, also doing a quick change to the API to move the blocking_io_executor parameter to the TermManager. It seems like TermManager already exposes a shutdown method for controlling lifecycles, so it would be nice to have the lifecycle of autocreated executors managed there. Should have this updated in a few minutes...

KoopaKing commented 1 year ago

Ok, for real though this is my last time trying to make the linter happy...

blink1073 commented 1 year ago

I was a bit concerned since technically making on_message a coroutine it is a breaking change. I added a downstream test for jupyter_server_terminals.

blink1073 commented 1 year ago

I think we're good, those tests pass. I tried testing spyder-terminal but it has a complicated setup for testing, and it overrides on_message anyway.

blink1073 commented 1 year ago

I'll cut a release with this tomorrow.

blink1073 commented 1 year ago

Done! https://github.com/jupyter/terminado/releases/tag/v0.16.0