Copilot commented on code in PR #13002:
URL: https://github.com/apache/cloudstack/pull/13002#discussion_r3067711457
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java:
##########
@@ -45,6 +45,13 @@ public ConsoleProxyGCThread(Map<String, ConsoleProxyClient>
connMap, Set<String>
this.removedSessionsSet = removedSet;
}
+ private int getMaxSessionIdleSeconds() {
+ if (ConsoleProxy.sessionTimeoutMillis <= 0) {
+ return DEFAULT_MAX_SESSION_IDLE_SECONDS;
+ }
+ return ConsoleProxy.sessionTimeoutMillis / 1000;
Review Comment:
`getMaxSessionIdleSeconds()` derives seconds via integer division
(`sessionTimeoutMillis / 1000`). If `consoleproxy.session.timeout` is
configured to a value between 1 and 999 ms, this returns 0, causing all
sessions to be considered idle immediately and dropped on the next GC scan.
Consider computing the idle threshold in milliseconds (compare
millis-to-millis) or clamping the derived seconds to at least 1 when
`sessionTimeoutMillis > 0`.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -125,6 +125,10 @@ public void onConnect(final Session session) throws
IOException, InterruptedExce
}
try {
+ session.setIdleTimeout(ConsoleProxy.sessionTimeoutMillis);
+ logger.debug("Set noVNC WebSocket idle timeout to {} ms for
session UUID: {}.",
+ ConsoleProxy.sessionTimeoutMillis, sessionUuid);
+
Review Comment:
`session.setIdleTimeout(ConsoleProxy.sessionTimeoutMillis)` is called
unconditionally. If the configured value is <= 0 (or otherwise invalid), Jetty
may interpret it as 'no timeout' or reject it, while `ConsoleProxyGCThread`
currently treats <=0 as a fallback to a fixed default. Please align the
semantics by validating before applying the idle timeout (and using the same
meaning for <=0 across WebSocket + GC cleanup).
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java:
##########
@@ -45,6 +45,13 @@ public ConsoleProxyGCThread(Map<String, ConsoleProxyClient>
connMap, Set<String>
this.removedSessionsSet = removedSet;
}
+ private int getMaxSessionIdleSeconds() {
+ if (ConsoleProxy.sessionTimeoutMillis <= 0) {
+ return DEFAULT_MAX_SESSION_IDLE_SECONDS;
+ }
+ return ConsoleProxy.sessionTimeoutMillis / 1000;
+ }
+
Review Comment:
New timeout derivation logic is not covered by unit tests. Since this module
already has tests, please add coverage for the configured timeout edge cases
(e.g., <=0, <1000 ms, normal values) to prevent regressions in session cleanup
behavior.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -379,7 +394,7 @@ public static void start(Properties conf) {
LOGGER.info("HTTP command port is disabled");
}
- ConsoleProxyGCThread cthread = new ConsoleProxyGCThread(connectionMap,
removedSessionsSet);
+ ConsoleProxyGCThread cthread = new ConsoleProxyGCThread(connectionMap,
removedSessionsSet /*, sessionTimeoutMillis */);
Review Comment:
The commented-out argument `/*, sessionTimeoutMillis */` suggests an
unfinished refactor and is confusing now that `ConsoleProxyGCThread` reads the
value statically. Please remove this comment to keep the startup path clean.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -165,6 +168,18 @@ private static void configProxy(Properties conf) {
defaultBufferSize = Integer.parseInt(s);
LOGGER.info("Setting defaultBufferSize=" + defaultBufferSize);
}
+
+ // New: read consoleproxy.session.timeout (milliseconds)
+ s = conf.getProperty("consoleproxy.session.timeout");
+ if (s != null) {
+ try {
+ sessionTimeoutMillis = Integer.parseInt(s);
+ LOGGER.info("Setting consoleproxy.session.timeout=" +
sessionTimeoutMillis);
+ } catch (NumberFormatException e) {
+ LOGGER.warn("Invalid value for consoleproxy.session.timeout: "
+ s +
+ ", using default " + sessionTimeoutMillis, e);
+ }
Review Comment:
`consoleproxy.session.timeout` is parsed and applied without validating the
range. Negative values (or 0, depending on intended semantics) can result in
unexpected behavior later (e.g., Jetty WebSocket idle timeout set to an
invalid/disabled value, and GC logic treating it differently). Consider
validating that the configured value is either a well-defined 'disabled'
sentinel or a positive minimum (e.g., >= 1000 ms), otherwise fall back to the
default.
--
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]
