-
Notifications
You must be signed in to change notification settings - Fork 664
KARAF-7174: Implement a SSH channel resource cleaner whiteboard#1884
KARAF-7174: Implement a SSH channel resource cleaner whiteboard #1884jbonofre wants to merge 1 commit intoapache:main from
Conversation
3db450f to
40a0e02
Compare
mattrpav
commented
Nov 12, 2024
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
commented
Nov 12, 2024
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
commented
Nov 12, 2024
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
commented
Nov 13, 2024
@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.
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.
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?
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.
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).
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.
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.
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.
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 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
commented
Nov 13, 2024
@mattrpav that's a good idea to leverage Closeable. Let me do an experiment.
No description provided.