You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Nikolay <no...@github.com> on 2015/06/17 22:02:38 UTC
[jclouds] Close connection after ExecChannel in sshj (#778)
ExecChannels created its own ssh connections that was not cleaned and SSH reader thread was leaked.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/778
-- Commit Summary --
* Close connection during ExecChannel
-- File Changes --
M drivers/sshj/src/main/java/org/jclouds/sshj/SshjSshClient.java (14)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/778.patch
https://github.com/jclouds/jclouds/pull/778.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Ignasi Barrera <no...@github.com>.
Cool! This started as an experiment and remained unfinished. Mind updating [this ugly javadoc](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/ssh/SshClient.java#L50-L53) too? Have you also checked if the SSHJ driver closes the connection properly?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-113471345
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Nikolay <no...@github.com>.
@nacx I didn't tried your code by hand, but I think we have different observations because in your example `channel` was not closed -- without my commit even manual closing of channel lead to leaked sessions.
If closing of channel sessions when context closed is desirable, then we should accumulate unclosed session objects in `SshjSshClient` and wipe them in `@PreDestroy public void disconnect()`. Does this makes sense?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-117298732
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Ignasi Barrera <no...@github.com>.
@chemikadze would you have some time to have a look at my comment?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-116850433
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Ignasi Barrera <no...@github.com>.
Do you have a minimal code snippet I can use to try this? I've tried the following code, but after closing the context the app is still running, because there are still some open resources left:
```java
try {
Template template = compute.templateBuilder()
.osFamily(OsFamily.CENTOS)
.smallest()
.build();
NodeMetadata node = Iterables.getOnlyElement(compute.createNodesInGroup("ssh", 1, template));
SshClient ssh = context.utils().sshForNode().apply(node);
ssh.connect();
BufferedReader in = null;
try {
ExecChannel channel = ssh.execChannel("yum -y update");
in = new BufferedReader(new InputStreamReader(channel.getOutput()));
String line = null;
while ((line = in.readLine()) != null) {
System.out.println("SSH> " + line);
}
} finally {
if (in != null) {
in.close();
}
ssh.disconnect();
}
} finally {
context.close();
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-114058372
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Nikolay <no...@github.com>.
@nacx We've noticed leaking "reader" threads without this, and with this patch threads are finishing correctly, I'll take a look what happens with connection itself.
No problem if you won't backport this to older branches.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-113495128
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Nikolay <no...@github.com>.
I like that approach, will try.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-117939027
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Nikolay <no...@github.com>.
@nacx I'll take a look today, sorry.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-117210463
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Ignasi Barrera <no...@github.com>.
I've just seen you opened this against 1.8.x and 1.7.x. There is no plan to release those branches anymore, so I'll merge this to master and 1.9.x when ready. Can you upgrade to the latest 1.9 version?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-113472647
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Ignasi Barrera <no...@github.com>.
Yes, it does.
To make sure everything is closed when closing the context, a nice approach would be to make the SSH clients also implement `Closeable` and let the close method call the disconnect one. This would allow us to modify the `create` method in the ssh client factory to register the ssh client in the jclouds [Closer](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/lifecycle/Closer.java), just the same way it is done [with the executors](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/concurrent/config/ExecutorServiceModule.java#L136).
That closer object is invoked when the context is closed, to make sure all resources bound to the context are properly closed. Registering the ssh clients there, to make sure everything is properly released even when the user has not explicitly called disconnect seems like a good idea. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-117341890
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Ignasi Barrera <no...@github.com>.
Closed #778.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#event-445021067
Re: [jclouds] Close connection after ExecChannel in sshj (#778)
Posted by Ignasi Barrera <no...@github.com>.
Ok. Finally got some time to properly test this and explore the mentioned variants. Apologies for the delay! I've successfully tested the patch and verified that all resources are now properly closed with both sshj and jsch drivers.
I've made some tests implementing the `Closer` approach but found these tests in the [sshj driver](https://github.com/jclouds/jclouds/blob/master/drivers/sshj/src/test/java/org/jclouds/sshj/SshjSshClientLiveTest.java#L173-L189) and the [jsch one](https://github.com/jclouds/jclouds/blob/master/drivers/jsch/src/test/java/org/jclouds/ssh/jsch/JschSshClientLiveTest.java#L263-L280) that suggest that it was a design decision not to close the SSH vlients when closing the context. We could discuss whether that is convenient or not (a valid use case could be to just get a connection to the node, then forget about the context and all Guice related resources), but not in the scope of this fix.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/060b66a4) and [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/c6f47ca9). Thanks for the patch @chemikadze!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/778#issuecomment-150987471