KARAF-7174: Implement a SSH channel resource cleaner whiteboard

[jbonofre]

No description provided.

[mattrpav]

This is a great idea and useful feature. Esp for other commands that use connection pools, or other stateful resources.

One suggestion -- perhaps a rename for consistency?

CommandCloseListener?

[jbonofre]

Yes,
Good point about the name. My only comment is that (potentially) it could be used for other services than commands. But let's start with shell commands :)

[mattrpav]

Yes, Good point about the name. My only comment is that (potentially) it could be used for other services than commands. But let's start with shell commands :)

How do you see it working for other services? I think things like scr services should use deactivate(). I'm struggling to find a use case where another service would go 'out-of-scope' so to speak similar to how the shell command can timeout.

[jbonofre]

@mattrpav I see a use case: a command use a service which starts a thread. The service has to be hook with ssh session to stop the thread when the session disconnects. So the service is not a shell command but it has to clean resources sync with ssh session.
SCR deactivate is a hook on SCR component lifecycle not the ssh session.

[mattrpav]

This appears to call .close() on every reference for every session that closes. Is that the correct association?

Wouldn't we want to close only the instance of the command associated with that session?

[jbonofre]

That's right it's a bit agressive: if we have several ssh clients on the Karaf ssh server, we will close all references whatever is the client.
It's pretty hard to "link" the session with the command (or maybe we can use a session property to identify the correct session).

[mattrpav]

Sounds good, a SSH sessionId stored in the command session state should do it.. then when the close loops, look for the matching one and close only that one.

Also, probably need a test with two sessions running.. first one aborts and second one stays active to confirm only the first one gets .close() called on it.

[mattrpav]

While we're at adding things to the Command/Action session.. this change is needed due to JDK removal of Security Manager: https://issues.apache.org/jira/browse/KARAF-7805

[mattrpav]

@mattrpav I see a use case: a command use a service which starts a thread. The service has to be hook with ssh session to stop the thread when the session disconnects. So the service is not a shell command but it has to clean resources sync with ssh session. SCR deactivate is a hook on SCR component lifecycle not the ssh session.

What about simply having Commands implement the JDK's built-in Closable interface and follow a convention vs marking the command with a domain-specific interface?

Seems like (commandInstance instanceof Closeable) would do the trick and we wouldn't need to create a new interface. Thoughts?

[jbonofre]

@mattrpav that's a good idea to leverage Closeable. Let me do an experiment.