Copilot commented on code in PR #13683:
URL: https://github.com/apache/cloudstack/pull/13683#discussion_r3642688410
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java:
##########
@@ -129,6 +129,28 @@ public void proxyMsgOverWebSocketConnection(ByteBuffer
msg) {
}
}
+ public void close() {
+ if (nioSocketConnection != null) {
+ nioSocketConnection.close();
+ }
+ if (webSocketReverseProxy != null) {
+ webSocketReverseProxy.close();
+ }
+ if (socket != null) {
+ try {
+ if (is != null) {
+ is.close();
+ }
+ if (os != null) {
+ os.close();
+ }
+ socket.close();
+ } catch (IOException e) {
+ logger.debug("Error closing socket: " + e.getMessage(), e);
+ }
+ }
+ }
Review Comment:
The socket/stream close logic is wrapped in a single try/catch, so if
closing `is` throws an IOException, `os` and `socket` won’t be closed (and the
method exits early). For a close path, it’s better to attempt closing each
resource independently (best-effort) so one failure doesn’t prevent releasing
the rest.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -209,13 +210,15 @@ public static ConsoleProxyAuthenticationResult
authenticateConsoleAccess(Console
}
String sessionUuid = param.getSessionUuid();
- if (allowedSessions.contains(sessionUuid)) {
- LOGGER.debug("Acquiring the session " + sessionUuid + " not
available for future use");
- allowedSessions.remove(sessionUuid);
- } else {
- LOGGER.info("Session " + sessionUuid + " has already been used,
cannot connect");
- authResult.setSuccess(false);
- return authResult;
+ synchronized (allowedSessionsLock) {
+ if (allowedSessions.contains(sessionUuid)) {
+ LOGGER.debug("Acquiring the session " + sessionUuid + " not
available for future use");
+ allowedSessions.remove(sessionUuid);
+ } else {
+ LOGGER.info("Session " + sessionUuid + " has already been
used, cannot connect");
+ authResult.setSuccess(false);
+ return authResult;
+ }
}
Review Comment:
Inside the one-time session block, doing `contains()` followed by `remove()`
is both slower (two lookups) and easy to get wrong if the synchronization
strategy changes later. `Set#remove` already returns whether the UUID was
present, so you can do the “check-and-consume” in one operation.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -374,6 +374,9 @@ public void closeClient() {
this.connectionAlive = false;
// Clear buffer reference to allow GC when client disconnects
this.readBuffer = null;
+ if (client != null) {
+ client.close();
+ }
ConsoleProxy.removeViewer(this);
Review Comment:
`closeClient()` now closes the underlying `NoVncClient` (which is the core
fix for lingering VNC sockets), but there’s no unit test asserting that the
client close is invoked. Given there are existing unit tests in this module,
adding a test (e.g., via Mockito injecting a mocked `NoVncClient`) would help
prevent regressions where sockets stop being closed again.
--
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]
