Skip to content

Commit b0bdff4

Browse files
Daniel-Pereira-LinuxDaniel-Pereira-Linux
committed
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
1 parent 92e44bb commit b0bdff4

4 files changed

Lines changed: 239 additions & 10 deletions

File tree

‎services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,14 @@ public void run() {
178178
}
179179
logger.info("Connection with client [{}] [IP: {}] is dead.", clientId, clientSourceIp);
180180
ConsoleProxy.grantReconnectWindowForSessionAndClientIp(sessionUuid, clientSourceIp);
181-
} catch (IOException e) {
182-
logger.error("Error on VNC client", e);
181+
} catch (IOException | RuntimeException e) {
182+
if (connectionAlive) {
183+
logger.error("Error on VNC client [{}] [IP: {}], closing the connection.", clientId, clientSourceIp, e);
184+
} else {
185+
logger.debug("VNC client [{}] [IP: {}] stopped after its connection was closed: {}", clientId, clientSourceIp, e.getMessage());
186+
}
187+
closeClient();
188+
session.close();
183189
}
184190
}
185191

@@ -253,8 +259,9 @@ protected void handshakeSecurityResult(int secType) {
253259

254260
client.processSecurityResultMsg();
255261
byte[] securityResultToClient = new byte[] { 0, 0, 0, 0 };
256-
sendMessageToVNCClient(securityResultToClient, 4);
262+
// Set the flag before sending, as noVNC may reply before sendMessageToVNCClient() returns
257263
client.setWaitForNoVnc(true);
264+
sendMessageToVNCClient(securityResultToClient, 4);
258265
}
259266

260267
protected int handshakeSecurity() {
@@ -288,8 +295,8 @@ private void initialisationPhase() {
288295
client.isTLSConnectionEstablished() ? "(TLS backend)" : "");
289296
byte[] bytesServerInit = rewriteServerNameInServerInit(serverInitByteArray, displayNameForVM);
290297

291-
sendMessageToVNCClient(bytesServerInit, bytesServerInit.length);
292298
client.setWaitForNoVnc(true);
299+
sendMessageToVNCClient(bytesServerInit, bytesServerInit.length);
293300
client.waitForNoVNCReply();
294301
}
295302

‎services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import java.security.spec.KeySpec;
3131
import java.util.Arrays;
3232
import java.util.List;
33+
import java.util.concurrent.TimeUnit;
3334

3435
import com.cloud.consoleproxy.util.RawHTTP;
3536
import com.cloud.consoleproxy.vnc.network.NioSocket;
@@ -59,6 +60,8 @@
5960
public class NoVncClient {
6061
protected Logger logger = LogManager.getLogger(getClass());
6162

63+
protected static final long NOVNC_REPLY_TIMEOUT_MS = 30000L;
64+
6265
private Socket socket;
6366
private DataInputStream is;
6467
private DataOutputStream os;
@@ -130,6 +133,10 @@ public void proxyMsgOverWebSocketConnection(ByteBuffer msg) {
130133
}
131134

132135
public void close() {
136+
synchronized (lock) {
137+
closed = true;
138+
lock.notifyAll();
139+
}
133140
if (nioSocketConnection != null) {
134141
nioSocketConnection.close();
135142
}
@@ -413,13 +420,32 @@ public ByteBuffer handshakeProtocolVersion() {
413420
return verStr;
414421
}
415422

423+
/**
424+
* Blocks until the noVNC client replies (see {@link #setWaitForNoVnc(boolean)}). Gives up with a
425+
* {@link CloudRuntimeException} if the client is closed or no reply arrives within {@link #NOVNC_REPLY_TIMEOUT_MS}.
426+
*/
416427
public void waitForNoVNCReply() {
417-
int cycles = 0;
418-
while (isWaitForNoVnc()) {
419-
cycles++;
420-
}
421-
if (logger.isDebugEnabled()) {
422-
logger.debug(String.format("Waited %d cycles for NoVnc", cycles));
428+
waitForNoVNCReply(NOVNC_REPLY_TIMEOUT_MS);
429+
}
430+
431+
protected void waitForNoVNCReply(long timeoutMs) {
432+
long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeoutMs);
433+
synchronized (lock) {
434+
while (waitForNoVnc) {
435+
if (closed) {
436+
throw new CloudRuntimeException("Connection closed while waiting for the noVNC reply");
437+
}
438+
long remainingMs = TimeUnit.NANOSECONDS.toMillis(deadline - System.nanoTime());
439+
if (remainingMs <= 0) {
440+
throw new CloudRuntimeException(String.format("Timed out after %d ms waiting for the noVNC reply", timeoutMs));
441+
}
442+
try {
443+
lock.wait(remainingMs);
444+
} catch (InterruptedException e) {
445+
Thread.currentThread().interrupt();
446+
throw new CloudRuntimeException("Interrupted while waiting for the noVNC reply", e);
447+
}
448+
}
423449
}
424450
}
425451

@@ -468,6 +494,7 @@ public int handshakeSecurityType() {
468494
public void setWaitForNoVnc(boolean val) {
469495
synchronized (lock) {
470496
this.waitForNoVnc = val;
497+
lock.notifyAll();
471498
}
472499
}
473500

@@ -478,6 +505,7 @@ public boolean isWaitForNoVnc() {
478505
}
479506

480507
private boolean waitForNoVnc = false;
508+
private boolean closed = false;
481509

482510
private Pair<Boolean, String> processSecurityResultType(int authResult) {
483511
boolean result = false;

‎services/console-proxy/server/src/test/java/com/cloud/consoleproxy/ConsoleProxyNoVncClientTest.java‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,25 @@
1616
// under the License.
1717
package com.cloud.consoleproxy;
1818

19+
import java.io.OutputStream;
20+
import java.net.InetAddress;
21+
import java.net.ServerSocket;
22+
import java.net.Socket;
23+
import java.nio.ByteBuffer;
24+
import java.nio.charset.StandardCharsets;
25+
import java.util.concurrent.CountDownLatch;
26+
import java.util.concurrent.TimeUnit;
27+
28+
import org.eclipse.jetty.websocket.api.RemoteEndpoint;
29+
import org.eclipse.jetty.websocket.api.Session;
1930
import org.junit.Assert;
2031
import org.junit.Test;
32+
import org.mockito.InOrder;
33+
import org.mockito.Mockito;
34+
import org.springframework.test.util.ReflectionTestUtils;
35+
36+
import com.cloud.consoleproxy.vnc.NoVncClient;
37+
import com.cloud.consoleproxy.vnc.RfbConstants;
2138

2239
public class ConsoleProxyNoVncClientTest {
2340
@Test
@@ -29,4 +46,87 @@ public void rewriteServerNameInServerInitTest() {
2946
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};
3047
Assert.assertArrayEquals(newServerInit, expectedBytes);
3148
}
49+
50+
@Test
51+
public void authenticateVNCServerThroughNioSocketExpectsNoVncReplyBeforeSendingTheMessage() throws Exception {
52+
Session session = Mockito.mock(Session.class);
53+
RemoteEndpoint remote = Mockito.mock(RemoteEndpoint.class);
54+
Mockito.when(session.getRemote()).thenReturn(remote);
55+
NoVncClient client = Mockito.mock(NoVncClient.class);
56+
Mockito.when(client.handshakeProtocolVersion()).thenReturn(ByteBuffer.wrap("RFB 003.008\n".getBytes(StandardCharsets.US_ASCII)));
57+
Mockito.when(client.handshakeSecurityType()).thenReturn(RfbConstants.NO_AUTH);
58+
Mockito.when(client.readServerInit()).thenReturn(new byte[24]);
59+
ConsoleProxyClientParam param = new ConsoleProxyClientParam();
60+
param.setClientDisplayName("vm");
61+
62+
ConsoleProxyNoVncClient viewer = new ConsoleProxyNoVncClient(session);
63+
ReflectionTestUtils.setField(viewer, "client", client);
64+
ReflectionTestUtils.setField(viewer, "clientParam", param);
65+
viewer.authenticateVNCServerThroughNioSocket();
66+
67+
// A reply flagged after the message is sent can arrive first and be lost
68+
InOrder inOrder = Mockito.inOrder(client, remote);
69+
inOrder.verify(client).processSecurityResultMsg();
70+
inOrder.verify(client).setWaitForNoVnc(true);
71+
inOrder.verify(remote).sendBytes(Mockito.argThat(msg -> msg.remaining() == 4));
72+
inOrder.verify(client).waitForNoVNCReply();
73+
inOrder.verify(client).readServerInit();
74+
inOrder.verify(client).setWaitForNoVnc(true);
75+
inOrder.verify(remote).sendBytes(Mockito.argThat(msg -> msg.remaining() > 24));
76+
inOrder.verify(client).waitForNoVNCReply();
77+
}
78+
79+
@Test(timeout = 20000)
80+
public void handshakeFailureClosesTheSession() throws Exception {
81+
try (ServerSocket vncServer = new ServerSocket(0, 1, InetAddress.getLoopbackAddress())) {
82+
Session session = Mockito.mock(Session.class);
83+
ConsoleProxyNoVncClient viewer = new ConsoleProxyNoVncClient(session);
84+
viewer.initClient(createClientParam(vncServer));
85+
86+
// The VNC server goes away before sending its protocol version
87+
vncServer.accept().close();
88+
89+
Mockito.verify(session, Mockito.timeout(10000)).close();
90+
Assert.assertFalse(viewer.isHostConnected());
91+
}
92+
}
93+
94+
@Test(timeout = 20000)
95+
public void closingTheViewerWhileWaitingForNoVncStopsTheWorker() throws Exception {
96+
try (ServerSocket vncServer = new ServerSocket(0, 1, InetAddress.getLoopbackAddress())) {
97+
Session session = Mockito.mock(Session.class);
98+
RemoteEndpoint remote = Mockito.mock(RemoteEndpoint.class);
99+
Mockito.when(session.getRemote()).thenReturn(remote);
100+
CountDownLatch versionSentToNoVnc = new CountDownLatch(1);
101+
Mockito.doAnswer(invocation -> {
102+
versionSentToNoVnc.countDown();
103+
return null;
104+
}).when(remote).sendBytes(Mockito.any(ByteBuffer.class));
105+
106+
ConsoleProxyNoVncClient viewer = new ConsoleProxyNoVncClient(session);
107+
viewer.initClient(createClientParam(vncServer));
108+
109+
try (Socket vncConnection = vncServer.accept()) {
110+
OutputStream out = vncConnection.getOutputStream();
111+
out.write("RFB 003.008\n".getBytes(StandardCharsets.US_ASCII));
112+
out.flush();
113+
Assert.assertTrue(versionSentToNoVnc.await(10, TimeUnit.SECONDS));
114+
115+
// noVNC never replies, then its websocket is closed (ConsoleProxyNoVNCHandler.onClose)
116+
viewer.closeClient();
117+
118+
Mockito.verify(session, Mockito.timeout(10000)).close();
119+
vncConnection.setSoTimeout(10000);
120+
Assert.assertEquals(-1, vncConnection.getInputStream().read());
121+
}
122+
}
123+
}
124+
125+
private static ConsoleProxyClientParam createClientParam(ServerSocket vncServer) {
126+
ConsoleProxyClientParam param = new ConsoleProxyClientParam();
127+
param.setClientHostAddress(vncServer.getInetAddress().getHostAddress());
128+
param.setClientHostPort(vncServer.getLocalPort());
129+
param.setClientIp("127.0.0.1");
130+
return param;
131+
}
32132
}

‎services/console-proxy/server/src/test/java/com/cloud/consoleproxy/vnc/NoVncClientTest.java‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package com.cloud.consoleproxy.vnc;
18+
19+
import com.cloud.utils.exception.CloudRuntimeException;
20+
import org.junit.Assert;
21+
import org.junit.Test;
22+
23+
public class NoVncClientTest {
24+
25+
@Test(timeout = 10000)
26+
public void waitForNoVNCReplyTimesOutWhenNoReplyArrives() {
27+
NoVncClient client = new NoVncClient();
28+
client.setWaitForNoVnc(true);
29+
long start = System.nanoTime();
30+
Assert.assertThrows(CloudRuntimeException.class, () -> client.waitForNoVNCReply(200));
31+
long elapsedMs = (System.nanoTime() - start) / 1000000;
32+
Assert.assertTrue("Gave up after " + elapsedMs + " ms", elapsedMs >= 150);
33+
}
34+
35+
@Test(timeout = 10000, expected = CloudRuntimeException.class)
36+
public void waitForNoVNCReplyFailsFastWhenAlreadyClosed() {
37+
NoVncClient client = new NoVncClient();
38+
client.close();
39+
client.setWaitForNoVnc(true);
40+
client.waitForNoVNCReply();
41+
}
42+
43+
@Test
44+
public void waitForNoVNCReplyReturnsImmediatelyWhenNotWaiting() {
45+
NoVncClient client = new NoVncClient();
46+
client.waitForNoVNCReply();
47+
}
48+
49+
@Test(timeout = 10000)
50+
public void waitForNoVNCReplyReturnsWhenReplyArrives() throws Exception {
51+
NoVncClient client = new NoVncClient();
52+
client.setWaitForNoVnc(true);
53+
Thread replier = new Thread(() -> {
54+
try {
55+
Thread.sleep(100);
56+
} catch (InterruptedException ignored) {
57+
Thread.currentThread().interrupt();
58+
}
59+
client.setWaitForNoVnc(false);
60+
});
61+
replier.start();
62+
client.waitForNoVNCReply();
63+
replier.join();
64+
Assert.assertFalse(client.isWaitForNoVnc());
65+
}
66+
67+
@Test(timeout = 10000, expected = CloudRuntimeException.class)
68+
public void waitForNoVNCReplyFailsWhenClientIsClosed() {
69+
NoVncClient client = new NoVncClient();
70+
client.setWaitForNoVnc(true);
71+
Thread closer = new Thread(() -> {
72+
try {
73+
Thread.sleep(100);
74+
} catch (InterruptedException ignored) {
75+
Thread.currentThread().interrupt();
76+
}
77+
client.close();
78+
});
79+
closer.start();
80+
client.waitForNoVNCReply();
81+
}
82+
83+
@Test(timeout = 10000, expected = CloudRuntimeException.class)
84+
public void waitForNoVNCReplyFailsWhenInterrupted() {
85+
NoVncClient client = new NoVncClient();
86+
client.setWaitForNoVnc(true);
87+
Thread.currentThread().interrupt();
88+
try {
89+
client.waitForNoVNCReply();
90+
} finally {
91+
Thread.interrupted();
92+
}
93+
}
94+
}

0 commit comments

Comments
 (0)