Copilot commented on code in PR #13002:
URL: https://github.com/apache/cloudstack/pull/13002#discussion_r3153122549
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java:
##########
@@ -32,9 +32,13 @@
* management software
*/
public class ConsoleProxyGCThread extends Thread {
- protected Logger logger = LogManager.getLogger(ConsoleProxyGCThread.class);
+ private static final Logger logger =
LogManager.getLogger(ConsoleProxyGCThread.class);
- private final static int MAX_SESSION_IDLE_SECONDS = 180;
+ /**
+ * Maximum time (in seconds) a console session is allowed to be idle
before it is closed.
+ * This value should be kept in sync with
ConsoleProxy.VIEWER_LINGER_SECONDS.
+ */
+ private static final int MAX_SESSION_IDLE_SECONDS = 180;
Review Comment:
`MAX_SESSION_IDLE_SECONDS` is still a hard-coded 180s constant. That doesn’t
match the documented default for `consoleproxy.session.timeout` (300000ms =
300s), and it also means the GC thread still cannot honor the configured
session timeout (the PR description mentions deriving this from a shared
`sessionTimeoutMillis` / getter). Please replace the constant usage with a
derived value based on the configured timeout (with a safe default/fallback) so
idle session cleanup reflects the global setting.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -142,25 +145,25 @@ private static void configProxy(Properties conf) {
factoryClzName = ConsoleProxyBaseServerFactoryImpl.class.getName();
}
- s = conf.getProperty("consoleproxy.httpCmdListenPort");
+ s = conf != null ? conf.getProperty("consoleproxy.httpCmdListenPort")
: null;
if (s != null) {
httpCmdListenPort = Integer.parseInt(s);
LOGGER.info("Setting httpCmdListenPort=" + s);
}
- s = conf.getProperty("consoleproxy.reconnectMaxRetry");
+ s = conf != null ? conf.getProperty("consoleproxy.reconnectMaxRetry")
: null;
if (s != null) {
reconnectMaxRetry = Integer.parseInt(s);
LOGGER.info("Setting reconnectMaxRetry=" + reconnectMaxRetry);
}
- s = conf.getProperty("consoleproxy.readTimeoutSeconds");
+ s = conf != null ? conf.getProperty("consoleproxy.readTimeoutSeconds")
: null;
if (s != null) {
readTimeoutSeconds = Integer.parseInt(s);
LOGGER.info("Setting readTimeoutSeconds=" + readTimeoutSeconds);
}
- s = conf.getProperty("consoleproxy.defaultBufferSize");
+ s = conf != null ? conf.getProperty("consoleproxy.defaultBufferSize")
: null;
if (s != null) {
defaultBufferSize = Integer.parseInt(s);
LOGGER.info("Setting defaultBufferSize=" + defaultBufferSize);
Review Comment:
This PR’s goal is to honor the global setting
`consoleproxy.session.timeout`, but `configProxy(Properties conf)` currently
does not read/apply any session timeout property (it only parses ports,
reconnectMaxRetry, readTimeoutSeconds, and buffer size). As a result, the
console-proxy JVM has no configured session timeout value to pass to the noVNC
WebSocket session / GC logic. Please add parsing/validation of the timeout (ms)
here (or in a dedicated config method) and store it in a shared field that can
be consumed by session cleanup and WebSocket idle timeouts.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -150,10 +151,11 @@ public void onConnect(final Session session) throws
IOException, InterruptedExce
if (queryMap.containsKey("extra")) {
param.setClientProvidedExtraSecurityToken(queryMap.get("extra"));
}
+
viewer = ConsoleProxy.getNoVncViewer(param, ajaxSessionIdStr,
session);
logger.info("Viewer has been created successfully [session UUID:
{}, client IP: {}].", sessionUuid, clientIp);
Review Comment:
The PR description says the Jetty WebSocket `Session` idle timeout should be
set from `consoleproxy.session.timeout` (via
`ConsoleProxy.sessionTimeoutMillis`), but `onConnect(...)` never configures the
WebSocket session timeout (e.g., `session.setIdleTimeout(...)`). Without that,
noVNC sessions may stay open indefinitely even when the global timeout is set.
Please apply the configured timeout to the Jetty session here (with a sensible
fallback when the setting is disabled/invalid).
--
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]
