Make terminal_class configurable and provide a manager to detect terminal status on linux systems

[Wh1isper]

#625

This is a temporary solution
In Jupyterhub, each input will trigger a record, which may have performance issues

[codecov-commenter]

Codecov Report

Merging #626 (82847b1) into master (e5cb6c4) will increase coverage by 0.12%.
The diff coverage is 93.27%.

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
+ Coverage   77.63%   77.75%   +0.12%     
==========================================
  Files         110      111       +1     
  Lines       10398    10509     +111     
  Branches     1408     1417       +9     
==========================================
+ Hits         8072     8171      +99     
- Misses       1929     1939      +10     
- Partials      397      399       +2     

Impacted Files	Coverage Δ
jupyter_server/terminal/handlers.py	80.64% <83.33%> (+4.64%)	⬆️
jupyter_server/terminal/terminalmanager.py	89.74% <92.10%> (+2.24%)	⬆️
jupyter_server/tests/test_stateful_terminal.py	94.44% <94.44%> (ø)
jupyter_server/serverapp.py	65.38% <100.00%> (+0.03%)	⬆️
jupyter_server/terminal/__init__.py	79.16% <100.00%> (ø)
jupyter_server/services/kernels/handlers.py	60.40% <0.00%> (-2.00%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5cb6c4...82847b1. Read the comment docs.

[blink1073]

Nice, thank you!

[Wh1isper]

well it cannot handle the following situations

$ cat untitled.py 
print(1)
while True:
    ...

python untitled.py
so I think I should add more detections

[Wh1isper]

This meets my needs, but may not be a good general approach
Maybe we need to raise an issue with terminado?

[blink1073]

As I understand it, it is terminal dependent, terminado is just reading from the pty: https://github.com/jupyter/terminado/blob/ab4ced0b77c8f198cdc27fe1d52c523635083461/terminado/websocket.py#L77

[Wh1isper]

Yes, it may be a little difficult to implement a reliable solution based on the current architecture. But we can delve into how to determine that an terminal is running

[kevin-bates]

Since this is introducing state management, it seems like the culling logic should be updated to no longer cull currently 'busy' terminals. Are we confident that the 'idle' state detection is reliable? If not, then culling could be adversely affected since (if I'm following things) a terminal could get "stuck" in 'busy' and we'd probably be better off leaving culling as-is.

[Wh1isper]

Since this is introducing state management, it seems like the culling logic should be updated to no longer cull currently 'busy' terminals. Are we confident that the 'idle' state detection is reliable? If not, then culling could be adversely affected since (if I'm following things) a terminal could get "stuck" in 'busy' and we'd probably be better off leaving culling as-is.

Yes, we'd probably be better off leaving culling as-is.
I am thinking about how to implement a reliable state determination. The current implementation requires the cooperation of the operating environment, in the environment provided by the unified is working properly.

My own idea is that this functionality is provided on-demand, and if someone needs to use it, the scenario is roughly the same as mine: detect if the user's kernel and terminal have been idle for a long time with a uniformly provided mirror and scheduling system (as jupyterhub). The point of adding terminal status detection is that last_activity_time is only updated on input and output and does not reflect the running state.

[Wh1isper]

@kevin-bates @blink1073
I have taken the TerminalManager with state storage and made it configurable in a separate class StatefulTerminalManager

If people use singleuser-based images, it performs well
In fact it shouldn't get stuck in a busy state as long as people don't switch users, bash styles or activate the virtual environment
c.ServerApp.stateful_terminals_enabled = True to enable it

[blink1073]

I tried locally to get it working on macOS, unsuccessfully. I think that perhaps this logic is too brittle to be in core, and this should be a third party Terminal manager, and we should allow the terminal manager to be configured similar to kernel_manager.

[Wh1isper]

I tried locally to get it working on macOS, unsuccessfully. I think that perhaps this logic is too brittle to be in core, and this should be a third party Terminal manager, and we should allow the terminal manager to be configured similar to kernel_manager.

Good idea , I've made the terminal_manager_class configurable
c.ServerApp.terminal_manager_class = 'jupyter_server.terminal.StatefulTerminalManager' to enable StatefulTerminalManager

[blink1073]

This seems reasonable to me as-is. Thanks @Wh1isper!

[blink1073]

Hi @Wh1isper, we discussed this PR in the weekly meeting. We think the right approach is to create a new repo and make terminals a server extension. This new class would then be a configurable part of that extension. I'll take the task of creating the repo and adding the initial content by the end of this month.

[Wh1isper]

Hi @Wh1isper, we discussed this PR in the weekly meeting. We think the right approach is to create a new repo and make terminals a server extension. This new class would then be a configurable part of that extension. I'll take the task of creating the repo and adding the initial content by the end of this month.

Great, thank you for your attention to this issue, I fully agree with this approach and will continue to follow the progress

[Wh1isper]

Turn to #640
Thanks all!