[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #242: Fix: close session when timeout during connect

[GitBox <gi...@apache.org>]

tomaswolf commented on code in PR #242:
URL: https://github.com/apache/mina-sshd/pull/242#discussion_r966772266


##########
sshd-core/src/main/java/org/apache/sshd/client/future/ConnectFuture.java:
##########
@@ -78,4 +80,13 @@ default ClientSession getClientSession() {
      * Cancels the connection attempt and notifies all threads waiting for this future.
      */
     void cancel();
+
+
+    /**
+     * Sets the newly connected session only if not timeout and notifies all threads waiting for this future. This method is invoked by SSHD
+     * internally. Please do not call this method directly.
+     *
+     * @param session The {@link ClientSession}

Review Comment:
   Missing `@throws` javadoc



##########
sshd-common/src/main/java/org/apache/sshd/common/future/DefaultSshFuture.java:
##########
@@ -79,6 +82,9 @@ protected Object await0(long timeoutMillis, boolean interruptable) throws Interr
                 }
 
                 curTime = System.currentTimeMillis();
+                if (curTime >= endTime) {
+                    setTimeout();
+                }

Review Comment:
   I think we also need to call setTimeout() above, in the exception handler, inside `if (interruptable) {`.



##########
sshd-common/src/main/java/org/apache/sshd/common/future/DefaultSshFuture.java:
##########
@@ -227,6 +251,13 @@ public void cancel() {
         setValue(CANCELED);
     }
 
+    private boolean isTimeout() {
+        return timeout;
+    }

Review Comment:
   Missing empty line between methods. Also make `isTimeout()` public; it may be useful in other contexts. And it must access the flag under lock.



##########
sshd-common/src/main/java/org/apache/sshd/common/future/DefaultSshFuture.java:
##########
@@ -50,6 +52,7 @@ public DefaultSshFuture(Object id, Object lock) {
         super(id);
 
         this.lock = (lock != null) ? lock : this;
+        this.timeout = false;

Review Comment:
   Unnecessary; it's the default value anyway.



##########
sshd-common/src/main/java/org/apache/sshd/common/future/DefaultSshFuture.java:
##########
@@ -112,6 +118,24 @@ public void setValue(Object newValue) {
         notifyListeners();
     }
 
+    /**
+     * Like setValue; but sets the value only if not timed out yet. Throws TimeoutException otherwise.
+     *
+     * @param newValue The operation result

Review Comment:
   Missing `@throws` javadoc.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org