Copilot commented on code in PR #11221:
URL: https://github.com/apache/cloudstack/pull/11221#discussion_r2212480866
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -316,9 +334,29 @@ private void setClientParam(ConsoleProxyClientParam param)
{
this.clientParam = param;
}
+ private ByteBuffer getOrCreateReadBuffer() {
+ if (readBuffer == null) {
+ readBuffer = ByteBuffer.allocate(ConsoleProxy.defaultBufferSize);
+ logger.debug("Allocated {} KB read buffer for client [{}]",
ConsoleProxy.defaultBufferSize / 1024 , clientId);
+
+ // Only apply batching logic for TLS connections to work around
16KB record limitation
+ // For non-TLS connections, use immediate flush for better
responsiveness
+ if (client != null && client.isTLSConnectionEstablished()) {
+ flushThreshold = Math.min(ConsoleProxy.defaultBufferSize / 4,
2048);
Review Comment:
The magic number 2048 for flush threshold should be extracted as a named
constant to improve code maintainability and make the batching strategy more
transparent.
```suggestion
flushThreshold = Math.min(ConsoleProxy.defaultBufferSize /
4, DEFAULT_FLUSH_THRESHOLD);
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -109,39 +112,54 @@ public void run() {
connectClientToVNCServer(tunnelUrl, tunnelSession,
websocketUrl);
authenticateToVNCServer(clientSourceIp);
- int readBytes;
- byte[] b;
+ // Track consecutive iterations with no data and sleep
accordingly. Only used for NIO socket connections.
+ int consecutiveZeroReads = 0;
+ int sleepTime = 1;
while (connectionAlive) {
logger.trace("Connection with client [{}] [IP: {}] is
alive.", clientId, clientSourceIp);
if (client.isVncOverWebSocketConnection()) {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
connectionAlive = session.isOpen();
+ sleepTime = 1;
} else if (client.isVncOverNioSocket()) {
- byte[] bytesArr;
- int nextBytes = client.getNextBytes();
- bytesArr = new byte[nextBytes];
- client.readBytes(bytesArr, nextBytes);
- logger.trace("Read [{}] bytes from client [{}].",
nextBytes, clientId);
- if (nextBytes > 0) {
-
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
+ ByteBuffer buffer = getOrCreateReadBuffer();
+ int bytesRead =
client.readAvailableDataIntoBuffer(buffer, buffer.remaining());
+
+ if (bytesRead > 0) {
updateFrontEndActivityTime();
+ consecutiveZeroReads = 0; // Reset counter on
successful read
+
+ sleepTime = 0; // Still no sleep to catch any
remaining data quickly
} else {
connectionAlive = session.isOpen();
+ consecutiveZeroReads++;
+ // Use adaptive sleep time to prevent
excessive busy waiting
+ sleepTime = Math.min(consecutiveZeroReads,
10); // Cap at 10ms max
Review Comment:
The magic number 10 for maximum sleep time should be extracted as a named
constant to improve readability and maintainability.
```suggestion
sleepTime = Math.min(consecutiveZeroReads,
MAX_SLEEP_TIME_MS); // Cap at 10ms max
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -316,9 +334,29 @@ private void setClientParam(ConsoleProxyClientParam param)
{
this.clientParam = param;
}
+ private ByteBuffer getOrCreateReadBuffer() {
+ if (readBuffer == null) {
+ readBuffer = ByteBuffer.allocate(ConsoleProxy.defaultBufferSize);
+ logger.debug("Allocated {} KB read buffer for client [{}]",
ConsoleProxy.defaultBufferSize / 1024 , clientId);
Review Comment:
There is an extra space in the log message: 'Allocated {} KB' should be
'Allocated {} KB'.
```suggestion
logger.debug("Allocated {} KB read buffer for client [{}]",
ConsoleProxy.defaultBufferSize / 1024 , clientId);
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/network/NioSocketSSLEngineManager.java:
##########
@@ -155,22 +157,25 @@ public int read(ByteBuffer data) throws IOException {
}
public int write(ByteBuffer data) throws IOException {
- int n = 0;
+ int totalBytesConsumed = 0;
+ int sessionAppBufSize = engine.getSession().getApplicationBufferSize();
+ boolean shouldBatch = ConsoleProxy.defaultBufferSize >
sessionAppBufSize;
Review Comment:
[nitpick] The batching decision logic could be extracted to a separate
method or made configurable. This hardcoded comparison makes the batching
behavior less transparent and harder to tune.
```suggestion
boolean shouldBatch = shouldBatchData(sessionAppBufSize);
```
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
