necouchman commented on code in PR #1117:
URL: https://github.com/apache/guacamole-client/pull/1117#discussion_r2479189145
##########
guacamole/src/main/java/org/apache/guacamole/GuacamoleSession.java:
##########
@@ -244,9 +246,11 @@ public Map<String, UserTunnel> getTunnels() {
*
* @param tunnel The tunnel to associate with this session.
*/
+
Review Comment:
There should not be a space between function documentation and the start of
the function itself.
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/HashTokenSessionMap.java:
##########
@@ -66,6 +66,18 @@ public class HashTokenSessionMap implements TokenSessionMap {
};
+ /**
+ * The connection timeout for individual Guacamole connections, in minutes.
+ * If 0, connections will not be automatically terminated based on age.
+ */
+ private final IntegerGuacamoleProperty CONNECTION_TIMEOUT =
+ new IntegerGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "connection-timeout"; }
+
+ };
+
Review Comment:
I'm thinking that we may want to name this slightly differently. "timeout"
carries with it the notion that the connection is waiting for something - user
input, or a response from a remote system, etc. - and what you've really
implemented, here, is a maximum connection duration, at which point the
connection(s) will be forcibly dropped and users will have to reconnect. My
thought is that maybe this code - both internally within the code and for
exposed things like properties - should call this something different -
`maximum-session-duration` or `session-limit` or something similar?
##########
guacamole/src/main/java/org/apache/guacamole/GuacamoleSession.java:
##########
@@ -24,6 +24,7 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
+
Review Comment:
Please remove this extra line - our current style does not separate the
`import` statements like this, and I don't think this is the place to start
that :-).
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/HashTokenSessionMap.java:
##########
@@ -85,10 +98,25 @@ public HashTokenSessionMap(Environment environment) {
logger.debug("Error while reading session timeout value.", e);
sessionTimeoutValue = 60;
}
+
+ // Read connection timeout from guacamole.properties
+ try {
+ connectionTimeoutValue =
environment.getProperty(CONNECTION_TIMEOUT, 0); // Disabled by default
+ }
+ catch (GuacamoleException e) {
+ logger.error("Unable to read guacamole.properties: {}",
e.getMessage());
+ logger.debug("Error while reading connection timeout value.", e);
+ connectionTimeoutValue = 0;
Review Comment:
Even though I hate CoPilot with hell fury, it's right, here, there should
not be an extra space.
##########
guacamole-common/src/main/java/org/apache/guacamole/net/GuacamoleTunnel.java:
##########
@@ -121,4 +121,11 @@ public interface GuacamoleTunnel {
*/
boolean isOpen();
+ /**
+ * Returns the creation time of this GuacamoleTunnel.
+ *
+ * @return The creation time of this GuacamoleTunnel.
Review Comment:
I know you're just following the style used in this particular Java file,
but, overall, we've moved to the following, and code additions follow this
style:
```
/**
* Returns the creation time of this GuacamoleTunnel
*
* @return
* The creation time of this GuacamoleTunnel.
*/
```
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/HashTokenSessionMap.java:
##########
@@ -85,10 +98,25 @@ public HashTokenSessionMap(Environment environment) {
logger.debug("Error while reading session timeout value.", e);
sessionTimeoutValue = 60;
}
+
+ // Read connection timeout from guacamole.properties
+ try {
+ connectionTimeoutValue =
environment.getProperty(CONNECTION_TIMEOUT, 0); // Disabled by default
+ }
+ catch (GuacamoleException e) {
+ logger.error("Unable to read guacamole.properties: {}",
e.getMessage());
+ logger.debug("Error while reading connection timeout value.", e);
+ connectionTimeoutValue = 0;
+ }
// Check for expired sessions every minute
logger.info("Sessions will expire after {} minutes of inactivity.",
sessionTimeoutValue);
- executor.scheduleAtFixedRate(new
SessionEvictionTask(sessionTimeoutValue * 60000l), 1, 1, TimeUnit.MINUTES);
+ if (connectionTimeoutValue > 0) {
+ logger.info("Connections will be terminated after {} minutes
regardless of activity.", connectionTimeoutValue);
+ } else {
Review Comment:
Please don't cuddle these lines...
```
}
else {
```
##########
guacamole/src/main/java/org/apache/guacamole/GuacamoleSession.java:
##########
@@ -284,6 +293,94 @@ public long getLastAccessedTime() {
return lastAccessedTime;
}
+ /**
+ * Returns the creation time of the specified tunnel, as the number of
+ * milliseconds since midnight January 1, 1970 GMT. Invoking this function
+ * does not affect the last access time of this session.
+ *
+ * @param uuid
+ * The UUID of the tunnel to get the creation time for.
+ *
+ * @return
+ * The time the tunnel was created, or 0 if the tunnel doesn't exist.
+ */
+ public long getTunnelCreationTime(String uuid) {
+ Long creationTime = tunnelCreationTimes.get(uuid);
+ return creationTime != null ? creationTime : 0;
+ }
+
+ /**
+ * Returns the age of the oldest tunnel in this session, in milliseconds.
+ * If no tunnels exist, returns 0. Invoking this function does not affect
+ * the last access time of this session.
+ *
+ * @return
+ * The age of the oldest tunnel in milliseconds, or 0 if no tunnels
exist.
+ */
+ public long getOldestTunnelAge() {
+ if (tunnelCreationTimes.isEmpty()) {
+ return 0;
+ }
+
+ long currentTime = System.currentTimeMillis();
+ long oldestCreationTime = tunnelCreationTimes.values().stream()
+ .mapToLong(Long::longValue)
+ .min()
+ .orElse(currentTime);
+
+ return currentTime - oldestCreationTime;
+ }
+
+ /**
+ * Closes and removes any tunnels in this session that exceed the specified
+ * age limit. Invoking this function does not affect the last access time
+ * of this session.
+ *
+ * @param maxAge
+ * The maximum allowed age of tunnels in milliseconds. Tunnels older
+ * than this will be closed and removed.
+ *
+ * @return
+ * The number of tunnels that were closed and removed.
+ */
+ public int closeExpiredTunnels(long maxAge) {
+ if (maxAge <= 0 || tunnelCreationTimes.isEmpty()) {
+ return 0;
+ }
+
+ long currentTime = System.currentTimeMillis();
+ int closedCount = 0;
+
+ // Find tunnels that exceed the age limit
+ List<String> expiredTunnelIds = new ArrayList<>();
+ for (Map.Entry<String, Long> entry : tunnelCreationTimes.entrySet()) {
+ long tunnelAge = currentTime - entry.getValue();
+ if (tunnelAge >= maxAge) {
+ expiredTunnelIds.add(entry.getKey());
+ }
+ }
+
+ // Close and remove expired tunnels
+ for (String tunnelId : expiredTunnelIds) {
+ UserTunnel tunnel = tunnels.get(tunnelId);
+ if (tunnel != null) {
+ try {
+ tunnel.close();
+ logger.debug("Closed tunnel \"{}\" due to connection
timeout.", tunnelId);
+ }
+ catch (GuacamoleException e) {
+ logger.debug("Unable to close expired tunnel \"" +
tunnelId + "\".", e);
+ }
+ // Remove from both maps regardless of whether close succeeded
+ tunnels.remove(tunnelId);
+ tunnelCreationTimes.remove(tunnelId);
+ closedCount++;
+ }
+ }
Review Comment:
I'm not sure I agree with CoPilot, here; however, is there any reason not to
use the `removeTunnel()` method?
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/HashTokenSessionMap.java:
##########
@@ -104,16 +132,26 @@ private class SessionEvictionTask implements Runnable {
*/
private final long sessionTimeout;
+ /**
+ * The maximum allowed age of any connection, in milliseconds.
+ * If 0, connections will not be terminated based on age.
+ */
+ private final long connectionTimeout;
+
/**
* Creates a new task which automatically evicts sessions which are
* older than the specified timeout, or are marked as invalid by an
* extension.
*
* @param sessionTimeout The maximum age of any session, in
* milliseconds.
+ * @param connectionTimeout The maximum age of any connection, in
+ * milliseconds. If 0, connections will not be
+ * terminated based on age.
Review Comment:
As with the note in the above file, this should follow the newer style that
most additions and updates are using:
```
* @param sessionTimeout
* The maximum age of any session, in milliseconds
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
