Add ability to cull terminals and track last activity

[kevin-bates]

This adds functionality to track a terminal's last activity and optionally cull terminals that have been inactive for some specified duration. Similar to culling kernels, culling terminals is configured by setting the parameter --TerminalManager.cull_inactive_timeout to a value greater than 0 (default is 0 - not enabled).

The implementation introduces a TerminalManager class which is a subclass of terminado's NamedTermManager class. The handlers have been updated to work with this terminal manager.

Here are the configurable options of the TerminalManager class...

TerminalManager options
-----------------------
--TerminalManager.cull_inactive_timeout=<Int>
    Default: 0
    Timeout (in seconds) in which a terminal has been inactive and ready to be
    culled. Values of 0 or lower disable culling.
--TerminalManager.cull_interval=<Int>
    Default: 300
    The interval (in seconds) on which to check for terminals exceeding the
    inactive timeout value.

This change also updates the REST API such that both name and last_activity are returned in the JSON response. As a result, this may be helpful in addressing #1834.

I don't see any issues in Notebook's display and handling of terminals with the additional field and I'm assuming it's just ignored. However, this kind of thing may be considered an API break, although, given its only in responses, most applications can tolerate this.

Resolves: #3868

[kevin-bates]

I should also note that I didn't introduce any kind of execution_state because it seems the code is more of an echo-er and I believe you'd need to introduce a more formal protocol - like that of kernels. As a result, if a given operation were performed within a terminal session and that operation produced no output for the more than the duration of the cull_inactive_timeout, that terminal session would be culled mid-operation. This behavior is analogous to kernel culling where both cull_connected and cull_busy are True. Caveat emptor.

[kevin-bates]

Moving to WIP to add auto-shutdown capabilities similar to kernels when the server is requested to shutdown.

[Zsailer]

@kevin-bates, this looks great to me (I love how clean the Terminal Manager API is). Just some minor, nitpicky comments.

Are you planning to write tests that check the culling mechanism?

Currently, there are no unit tests for terminals (probably because they are often flaky), and terminals won't affect the main app, so I'm fine merging without tests. We added terminal tests in jupyter_server, so we can port this PR there and write simple tests over there. Let me know what you think.

[kevin-bates]

Thanks for the review @Zsailer. You're right about the test (or lack of). Since there weren't any previously I didn't go down that road for this but would be willing to look into this in jupyter_server (if that's okay for the time being).

I'll address the other issues in an update.

[kevin-bates]

@Zsailer - I've gone ahead and added some basic testing of the new terminal manager and culling. I then noticed (and should have remembered) that jupyter_server has some terminal tests already (awesome) - so those tests can be updated when this is ported to JS.

I suspect the reason there weren't tests in notebook is because this functionality is getting tested in terminado, so I think it's sufficient with just exercising the terminal manager functionality right now.

[Zsailer]

@kevin-bates, the tests for the TerminalManager look great! Thank you for crafting those.

(Jupyter Server needed some terminal REST API tests, since were passing some extra params through the REST API.)

[Zsailer]

I don't see any issues in Notebook's display and handling of terminals with the additional field and I'm assuming it's just ignored. However, this kind of thing may be considered an API break, although, given its only in responses, most applications can tolerate this.

I think a short (separate) section in the changelog mentioning that a REST API change happened should be enough to cover this "breaking" change. (Like you) I assume that most Jupyter client/frontend applications don't validate every field, so the extra fields will be ignored.

[Zsailer]

This looks good to me! Thanks, @kevin-bates.

[takluyver]

BTW, was it deliberate that this made terminado a hard requirement? Previously, it would catch an ImportError and the UI would show that terminals were unavailable. Now notebookapp can't be imported without terminado available. See #5462.

[kevin-bates]

hi Thomas - it's great to hear from you.

No, it wasn't deliberate by any means - that should be evident by looking at the changeset. The act of wrapping terminado in a local manager class, coupled with providing users help options for enabling culling, inadvertently (and unknowingly) affected the runtime handling of teminado's presence. I will make this my focus today.

[Zsailer]

Oops, thanks, @takluyver. I didn’t know we made terminado an optional dependency either. Apologies for missing this in the review, Kevin.

[meeseeksmachine]

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/terminal-manager-cull-properties-not-being-applied-to-notebook-config/6222/2