Fix resource leaking and make the code more testable #41
Conversation
|
@peterjhill please review |
| private int port; | ||
| private int connectionTimeout; | ||
| private int commandTimeout; | ||
| private final JSch sshClient; |
There was a problem hiding this comment.
good call making all these final
|
|
||
| this.sshClient = new JSch(); | ||
| this.sshClient = (sshClient != null) ? sshClient : new JSch(); |
There was a problem hiding this comment.
love using ternary expressions..
| @@ -189,8 +187,6 @@ private String createHelloRPC(List<String> capabilities) { | |||
| */ | |||
| private NetconfSession createNetconfSession() throws NetconfException { | |||
| if (!isConnected()) { | |||
There was a problem hiding this comment.
it looks like we don't do anything interesting in this method anymore, now that you in line 138 do the
this.sshClient = (sshClient != null) ? sshClient : new JSch();
should we make the createNeconfSession a no-op and deprecate it?
There was a problem hiding this comment.
We use createNetconfSession in connect() method and sshClient initialization is a different thing than session creation. no-op means no-operations, correct?
There was a problem hiding this comment.
ah ha! when i was looking at the github ui, it looked like the create netconf session method code ended below line 192.
got it!. 👍
There was a problem hiding this comment.
oh, yeah. GitHub sometimes is very confusing
There was a problem hiding this comment.
Looks great!
@ydnath looks good to me..
Thanks @Vladislav-Kisliy
| @@ -189,8 +187,6 @@ private String createHelloRPC(List<String> capabilities) { | |||
| */ | |||
| private NetconfSession createNetconfSession() throws NetconfException { | |||
| if (!isConnected()) { | |||
There was a problem hiding this comment.
ah ha! when i was looking at the github ui, it looked like the create netconf session method code ended below line 192.
got it!. 👍
isConnectedmethod doesn't cover all cases. For example:If a device has opened the ssh port however doesn't have netconf subsystem, a connection gets stuck for a long period of time.