Copilot commented on code in PR #13058:
URL: https://github.com/apache/cloudstack/pull/13058#discussion_r3309508191
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -93,15 +94,16 @@ public void onConnect(final Session session) throws
IOException, InterruptedExce
String websocketUrl = queryMap.get("websocketUrl");
String sessionUuid = queryMap.get("sessionUuid");
String clientIp =
session.getRemoteAddress().getAddress().getHostAddress();
+ boolean sessionRequiresNewViewer =
Boolean.parseBoolean(queryMap.get("sessionRequiresNewViewer"));
- if (tag == null)
+ if (tag == null) {
tag = "";
+ }
Review Comment:
`sessionRequiresNewViewer` is parsed from the query string but never used.
This is confusing (and conflicts with the PR description about removing
newer-branch references). Please remove the unused variable/parameter, or wire
it into the viewer selection logic if it is required on 4.20.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java:
##########
@@ -16,98 +16,119 @@
// under the License.
package com.cloud.consoleproxy;
+
import java.io.File;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
+
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.LogManager;
+
/**
*
* ConsoleProxyGCThread does house-keeping work for the process, it helps
cleanup log files,
* recycle idle client sessions without front-end activities and report client
stats to external
* 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;
private final Map<String, ConsoleProxyClient> connMap;
private final Set<String> removedSessionsSet;
private long lastLogScan = 0;
+
public ConsoleProxyGCThread(Map<String, ConsoleProxyClient> connMap,
Set<String> removedSet) {
this.connMap = connMap;
this.removedSessionsSet = removedSet;
}
+
private void cleanupLogging() {
- if (lastLogScan != 0 && System.currentTimeMillis() - lastLogScan <
3600000)
+ if (lastLogScan != 0 && System.currentTimeMillis() - lastLogScan <
3600000) {
return;
+ }
+
lastLogScan = System.currentTimeMillis();
+
File logDir = new File("./logs");
- File files[] = logDir.listFiles();
+ File[] files = logDir.listFiles();
if (files != null) {
for (File file : files) {
if (System.currentTimeMillis() - file.lastModified() >=
86400000L) {
try {
file.delete();
} catch (Throwable e) {
- logger.info("[ignored]"
- + "failed to delete file: " +
e.getLocalizedMessage());
+ logger.info("[ignored] failed to delete file: " +
e.getLocalizedMessage());
}
}
}
}
}
+
@Override
public void run() {
+
boolean bReportLoad = false;
long lastReportTick = System.currentTimeMillis();
+
while (true) {
cleanupLogging();
bReportLoad = false;
+
if (logger.isDebugEnabled()) {
- logger.debug(String.format("connMap=%s, removedSessions=%s",
connMap, removedSessionsSet));
+ logger.debug(String.format("ConsoleProxyGCThread loop:
connMap=%s, removedSessions=%s", connMap, removedSessionsSet));
}
- Set<String> e = connMap.keySet();
- Iterator<String> iterator = e.iterator();
+ Set<String> keys = connMap.keySet();
+ Iterator<String> iterator = keys.iterator();
while (iterator.hasNext()) {
String key;
ConsoleProxyClient client;
+
synchronized (connMap) {
key = iterator.next();
client = connMap.get(key);
}
- long seconds_unused = (System.currentTimeMillis() -
client.getClientLastFrontEndActivityTime()) / 1000;
- if (seconds_unused < MAX_SESSION_IDLE_SECONDS) {
+
+ if (client == null) {
+ continue;
+ }
+
+
+ long millisecondsUnused = System.currentTimeMillis() -
client.getClientLastFrontEndActivityTime();
+ if (millisecondsUnused < ConsoleProxy.sessionTimeoutMillis) {
continue;
}
+
synchronized (connMap) {
connMap.remove(key);
bReportLoad = true;
}
Review Comment:
The GC loop iterates `connMap.keySet().iterator()` but removes entries via
`connMap.remove(key)` while still using the iterator. This can throw
`ConcurrentModificationException` (and permanently stop the GC thread),
especially since other threads also modify `connMap`. Please iterate over a
snapshot of keys/entries or remove via `iterator.remove()` while holding the
appropriate lock, and keep the close operation outside the lock.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -53,23 +56,33 @@
public class ConsoleProxy {
protected static Logger LOGGER = LogManager.getLogger(ConsoleProxy.class);
+
public static final int KEYBOARD_RAW = 0;
public static final int KEYBOARD_COOKED = 1;
+
public static final int VIEWER_LINGER_SECONDS = 180;
+ // New: default and effective session timeout (milliseconds) honoured from
consoleproxy.session.timeout
+ public static final int DEFAULT_SESSION_TIMEOUT_MILLIS = 300000;
+ public static volatile int sessionTimeoutMillis =
DEFAULT_SESSION_TIMEOUT_MILLIS;
+
+
Review Comment:
The newly introduced `sessionTimeoutMillis` (ms) and the existing
`VIEWER_LINGER_SECONDS` (s) now represent two different “idle” thresholds used
in different places (e.g., `isFrontEndAlive()` vs GC cleanup). This can lead to
inconsistent behaviour where sessions are considered dead/reused at one timeout
but only cleaned up at another. Also, parsing/storing the timeout as an `int`
risks overflow for larger millisecond values. Consider using a single
long-based timeout source (e.g., `volatile long sessionTimeoutMillis`) and
deriving any second-based thresholds from it where needed.
--
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]
