From 40d527cf69736d61e0a4da2de878555f55f85680 Mon Sep 17 00:00:00 2001 From: Daniel Pereira Date: Fri, 30 Oct 2026 12:27:11 -0300 Subject: [PATCH] console-proxy: stop busy-spinning in NoVncClient.waitForNoVNCReply waitForNoVNCReply() looped on isWaitForNoVnc() with no sleep, timeout or check that the connection was still alive. If the VNC socket or the websocket was closed before noVNC replied (e.g. VM restarted while the console was connecting), the thread spun at 100% CPU forever. Wait on the existing lock instead, woken by setWaitForNoVnc() and close(), and fail with a CloudRuntimeException when the client is closed, the thread is interrupted, or no reply arrives within 30 seconds. Since the wait can now fail, the viewer thread closes the client and the session on any RuntimeException, and the wait flag is set before sending the message that noVNC replies to, so a fast reply is not missed. Fixes: #14538 --- .../consoleproxy/ConsoleProxyNoVncClient.java | 15 ++- .../cloud/consoleproxy/vnc/NoVncClient.java | 40 +++++-- .../ConsoleProxyNoVncClientTest.java | 100 ++++++++++++++++++ .../consoleproxy/vnc/NoVncClientTest.java | 94 ++++++++++++++++ 4 files changed, 239 insertions(+), 10 deletions(-) create mode 100644 services/console-proxy/server/src/test/java/com/cloud/consoleproxy/vnc/NoVncClientTest.java diff --git a/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java b/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java index 2ac85e5c5d33..19cfda960132 100644 --- a/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java +++ b/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java @@ -178,8 +178,14 @@ public void run() { } logger.info("Connection with client [{}] [IP: {}] is dead.", clientId, clientSourceIp); ConsoleProxy.grantReconnectWindowForSessionAndClientIp(sessionUuid, clientSourceIp); - } catch (IOException e) { - logger.error("Error on VNC client", e); + } catch (IOException | RuntimeException e) { + if (connectionAlive) { + logger.error("Error on VNC client [{}] [IP: {}], closing the connection.", clientId, clientSourceIp, e); + } else { + logger.debug("VNC client [{}] [IP: {}] stopped after its connection was closed: {}", clientId, clientSourceIp, e.getMessage()); + } + closeClient(); + session.close(); } } @@ -253,8 +259,9 @@ protected void handshakeSecurityResult(int secType) { client.processSecurityResultMsg(); byte[] securityResultToClient = new byte[] { 0, 0, 0, 0 }; - sendMessageToVNCClient(securityResultToClient, 4); + // Set the flag before sending, as noVNC may reply before sendMessageToVNCClient() returns client.setWaitForNoVnc(true); + sendMessageToVNCClient(securityResultToClient, 4); } protected int handshakeSecurity() { @@ -288,8 +295,8 @@ private void initialisationPhase() { client.isTLSConnectionEstablished() ? "(TLS backend)" : ""); byte[] bytesServerInit = rewriteServerNameInServerInit(serverInitByteArray, displayNameForVM); - sendMessageToVNCClient(bytesServerInit, bytesServerInit.length); client.setWaitForNoVnc(true); + sendMessageToVNCClient(bytesServerInit, bytesServerInit.length); client.waitForNoVNCReply(); } diff --git a/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java b/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java index 938b810356be..dfaaee821077 100644 --- a/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java +++ b/services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java @@ -30,6 +30,7 @@ import java.security.spec.KeySpec; import java.util.Arrays; import java.util.List; +import java.util.concurrent.TimeUnit; import com.cloud.consoleproxy.util.RawHTTP; import com.cloud.consoleproxy.vnc.network.NioSocket; @@ -59,6 +60,8 @@ public class NoVncClient { protected Logger logger = LogManager.getLogger(getClass()); + protected static final long NOVNC_REPLY_TIMEOUT_MS = 30000L; + private Socket socket; private DataInputStream is; private DataOutputStream os; @@ -130,6 +133,10 @@ public void proxyMsgOverWebSocketConnection(ByteBuffer msg) { } public void close() { + synchronized (lock) { + closed = true; + lock.notifyAll(); + } if (nioSocketConnection != null) { nioSocketConnection.close(); } @@ -413,13 +420,32 @@ public ByteBuffer handshakeProtocolVersion() { return verStr; } + /** + * Blocks until the noVNC client replies (see {@link #setWaitForNoVnc(boolean)}). Gives up with a + * {@link CloudRuntimeException} if the client is closed or no reply arrives within {@link #NOVNC_REPLY_TIMEOUT_MS}. + */ public void waitForNoVNCReply() { - int cycles = 0; - while (isWaitForNoVnc()) { - cycles++; - } - if (logger.isDebugEnabled()) { - logger.debug(String.format("Waited %d cycles for NoVnc", cycles)); + waitForNoVNCReply(NOVNC_REPLY_TIMEOUT_MS); + } + + protected void waitForNoVNCReply(long timeoutMs) { + long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeoutMs); + synchronized (lock) { + while (waitForNoVnc) { + if (closed) { + throw new CloudRuntimeException("Connection closed while waiting for the noVNC reply"); + } + long remainingMs = TimeUnit.NANOSECONDS.toMillis(deadline - System.nanoTime()); + if (remainingMs <= 0) { + throw new CloudRuntimeException(String.format("Timed out after %d ms waiting for the noVNC reply", timeoutMs)); + } + try { + lock.wait(remainingMs); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new CloudRuntimeException("Interrupted while waiting for the noVNC reply", e); + } + } } } @@ -468,6 +494,7 @@ public int handshakeSecurityType() { public void setWaitForNoVnc(boolean val) { synchronized (lock) { this.waitForNoVnc = val; + lock.notifyAll(); } } @@ -478,6 +505,7 @@ public boolean isWaitForNoVnc() { } private boolean waitForNoVnc = false; + private boolean closed = false; private Pair processSecurityResultType(int authResult) { boolean result = false; diff --git a/services/console-proxy/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyNoVncClientTest.java b/services/console-proxy/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyNoVncClientTest.java index 56a87373711b..afb87d880727 100644 --- a/services/console-proxy/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyNoVncClientTest.java +++ b/services/console-proxy/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyNoVncClientTest.java @@ -16,8 +16,25 @@ // under the License. package com.cloud.consoleproxy; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import org.eclipse.jetty.websocket.api.RemoteEndpoint; +import org.eclipse.jetty.websocket.api.Session; import org.junit.Assert; import org.junit.Test; +import org.mockito.InOrder; +import org.mockito.Mockito; +import org.springframework.test.util.ReflectionTestUtils; + +import com.cloud.consoleproxy.vnc.NoVncClient; +import com.cloud.consoleproxy.vnc.RfbConstants; public class ConsoleProxyNoVncClientTest { @Test @@ -29,4 +46,87 @@ public void rewriteServerNameInServerInitTest() { byte[] expectedBytes = new byte[]{4, 0, 3, 0, 32, 24, 0, 1, 0, -1, 0, -1, 0, -1, 16, 8, 0, 0, 0, 0, 0, 0, 0, 22, 115, 101, 114, 118, 101, 114, 49, 50, 51, 44, 32, 98, 97, 99, 107, 101, 110, 100, 58, 84, 76, 83}; Assert.assertArrayEquals(newServerInit, expectedBytes); } + + @Test + public void authenticateVNCServerThroughNioSocketExpectsNoVncReplyBeforeSendingTheMessage() throws Exception { + Session session = Mockito.mock(Session.class); + RemoteEndpoint remote = Mockito.mock(RemoteEndpoint.class); + Mockito.when(session.getRemote()).thenReturn(remote); + NoVncClient client = Mockito.mock(NoVncClient.class); + Mockito.when(client.handshakeProtocolVersion()).thenReturn(ByteBuffer.wrap("RFB 003.008\n".getBytes(StandardCharsets.US_ASCII))); + Mockito.when(client.handshakeSecurityType()).thenReturn(RfbConstants.NO_AUTH); + Mockito.when(client.readServerInit()).thenReturn(new byte[24]); + ConsoleProxyClientParam param = new ConsoleProxyClientParam(); + param.setClientDisplayName("vm"); + + ConsoleProxyNoVncClient viewer = new ConsoleProxyNoVncClient(session); + ReflectionTestUtils.setField(viewer, "client", client); + ReflectionTestUtils.setField(viewer, "clientParam", param); + viewer.authenticateVNCServerThroughNioSocket(); + + // A reply flagged after the message is sent can arrive first and be lost + InOrder inOrder = Mockito.inOrder(client, remote); + inOrder.verify(client).processSecurityResultMsg(); + inOrder.verify(client).setWaitForNoVnc(true); + inOrder.verify(remote).sendBytes(Mockito.argThat(msg -> msg.remaining() == 4)); + inOrder.verify(client).waitForNoVNCReply(); + inOrder.verify(client).readServerInit(); + inOrder.verify(client).setWaitForNoVnc(true); + inOrder.verify(remote).sendBytes(Mockito.argThat(msg -> msg.remaining() > 24)); + inOrder.verify(client).waitForNoVNCReply(); + } + + @Test(timeout = 20000) + public void handshakeFailureClosesTheSession() throws Exception { + try (ServerSocket vncServer = new ServerSocket(0, 1, InetAddress.getLoopbackAddress())) { + Session session = Mockito.mock(Session.class); + ConsoleProxyNoVncClient viewer = new ConsoleProxyNoVncClient(session); + viewer.initClient(createClientParam(vncServer)); + + // The VNC server goes away before sending its protocol version + vncServer.accept().close(); + + Mockito.verify(session, Mockito.timeout(10000)).close(); + Assert.assertFalse(viewer.isHostConnected()); + } + } + + @Test(timeout = 20000) + public void closingTheViewerWhileWaitingForNoVncStopsTheWorker() throws Exception { + try (ServerSocket vncServer = new ServerSocket(0, 1, InetAddress.getLoopbackAddress())) { + Session session = Mockito.mock(Session.class); + RemoteEndpoint remote = Mockito.mock(RemoteEndpoint.class); + Mockito.when(session.getRemote()).thenReturn(remote); + CountDownLatch versionSentToNoVnc = new CountDownLatch(1); + Mockito.doAnswer(invocation -> { + versionSentToNoVnc.countDown(); + return null; + }).when(remote).sendBytes(Mockito.any(ByteBuffer.class)); + + ConsoleProxyNoVncClient viewer = new ConsoleProxyNoVncClient(session); + viewer.initClient(createClientParam(vncServer)); + + try (Socket vncConnection = vncServer.accept()) { + OutputStream out = vncConnection.getOutputStream(); + out.write("RFB 003.008\n".getBytes(StandardCharsets.US_ASCII)); + out.flush(); + Assert.assertTrue(versionSentToNoVnc.await(10, TimeUnit.SECONDS)); + + // noVNC never replies, then its websocket is closed (ConsoleProxyNoVNCHandler.onClose) + viewer.closeClient(); + + Mockito.verify(session, Mockito.timeout(10000)).close(); + vncConnection.setSoTimeout(10000); + Assert.assertEquals(-1, vncConnection.getInputStream().read()); + } + } + } + + private static ConsoleProxyClientParam createClientParam(ServerSocket vncServer) { + ConsoleProxyClientParam param = new ConsoleProxyClientParam(); + param.setClientHostAddress(vncServer.getInetAddress().getHostAddress()); + param.setClientHostPort(vncServer.getLocalPort()); + param.setClientIp("127.0.0.1"); + return param; + } } diff --git a/services/console-proxy/server/src/test/java/com/cloud/consoleproxy/vnc/NoVncClientTest.java b/services/console-proxy/server/src/test/java/com/cloud/consoleproxy/vnc/NoVncClientTest.java new file mode 100644 index 000000000000..3387e097646d --- /dev/null +++ b/services/console-proxy/server/src/test/java/com/cloud/consoleproxy/vnc/NoVncClientTest.java @@ -0,0 +1,94 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.consoleproxy.vnc; + +import com.cloud.utils.exception.CloudRuntimeException; +import org.junit.Assert; +import org.junit.Test; + +public class NoVncClientTest { + + @Test(timeout = 10000) + public void waitForNoVNCReplyTimesOutWhenNoReplyArrives() { + NoVncClient client = new NoVncClient(); + client.setWaitForNoVnc(true); + long start = System.nanoTime(); + Assert.assertThrows(CloudRuntimeException.class, () -> client.waitForNoVNCReply(200)); + long elapsedMs = (System.nanoTime() - start) / 1000000; + Assert.assertTrue("Gave up after " + elapsedMs + " ms", elapsedMs >= 150); + } + + @Test(timeout = 10000, expected = CloudRuntimeException.class) + public void waitForNoVNCReplyFailsFastWhenAlreadyClosed() { + NoVncClient client = new NoVncClient(); + client.close(); + client.setWaitForNoVnc(true); + client.waitForNoVNCReply(); + } + + @Test + public void waitForNoVNCReplyReturnsImmediatelyWhenNotWaiting() { + NoVncClient client = new NoVncClient(); + client.waitForNoVNCReply(); + } + + @Test(timeout = 10000) + public void waitForNoVNCReplyReturnsWhenReplyArrives() throws Exception { + NoVncClient client = new NoVncClient(); + client.setWaitForNoVnc(true); + Thread replier = new Thread(() -> { + try { + Thread.sleep(100); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + } + client.setWaitForNoVnc(false); + }); + replier.start(); + client.waitForNoVNCReply(); + replier.join(); + Assert.assertFalse(client.isWaitForNoVnc()); + } + + @Test(timeout = 10000, expected = CloudRuntimeException.class) + public void waitForNoVNCReplyFailsWhenClientIsClosed() { + NoVncClient client = new NoVncClient(); + client.setWaitForNoVnc(true); + Thread closer = new Thread(() -> { + try { + Thread.sleep(100); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + } + client.close(); + }); + closer.start(); + client.waitForNoVNCReply(); + } + + @Test(timeout = 10000, expected = CloudRuntimeException.class) + public void waitForNoVNCReplyFailsWhenInterrupted() { + NoVncClient client = new NoVncClient(); + client.setWaitForNoVnc(true); + Thread.currentThread().interrupt(); + try { + client.waitForNoVNCReply(); + } finally { + Thread.interrupted(); + } + } +}