From fe07cf9b703e2aa9a83ed5ce57e51fe6cb33105d Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sat, 1 Sep 2018 19:19:08 -0700 Subject: [PATCH 1/5] GUACAMOLE-567: Move client instability state to own flag. Actual current connection state is lost otherwise. --- .../client/controllers/clientController.js | 2 +- .../webapp/app/client/types/ManagedClient.js | 8 ++-- .../app/client/types/ManagedClientState.js | 40 ++++++++++++++----- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/guacamole/src/main/webapp/app/client/controllers/clientController.js b/guacamole/src/main/webapp/app/client/controllers/clientController.js index ffbe3c539..41c6ba6b7 100644 --- a/guacamole/src/main/webapp/app/client/controllers/clientController.js +++ b/guacamole/src/main/webapp/app/client/controllers/clientController.js @@ -635,7 +635,7 @@ angular.module('client').controller('clientController', ['$scope', '$routeParams * otherwise. */ $scope.isConnectionUnstable = function isConnectionUnstable() { - return $scope.client && $scope.client.clientState.connectionState === ManagedClientState.ConnectionState.UNSTABLE; + return $scope.client && $scope.client.clientState.tunnelUnstable; }; // Show status dialog when connection status changes diff --git a/guacamole/src/main/webapp/app/client/types/ManagedClient.js b/guacamole/src/main/webapp/app/client/types/ManagedClient.js index 09c96a91e..a9bc3beac 100644 --- a/guacamole/src/main/webapp/app/client/types/ManagedClient.js +++ b/guacamole/src/main/webapp/app/client/types/ManagedClient.js @@ -346,16 +346,14 @@ angular.module('client').factory('ManagedClient', ['$rootScope', '$injector', ManagedClientState.ConnectionState.CONNECTING); break; - // Connection is established + // Connection is established / no longer unstable case Guacamole.Tunnel.State.OPEN: - ManagedClientState.setConnectionState(managedClient.clientState, - ManagedClientState.ConnectionState.CONNECTED); + ManagedClientState.setTunnelUnstable(managedClient.clientState, false); break; // Connection is established but misbehaving case Guacamole.Tunnel.State.UNSTABLE: - ManagedClientState.setConnectionState(managedClient.clientState, - ManagedClientState.ConnectionState.UNSTABLE); + ManagedClientState.setTunnelUnstable(managedClient.clientState, true); break; // Connection has closed diff --git a/guacamole/src/main/webapp/app/client/types/ManagedClientState.js b/guacamole/src/main/webapp/app/client/types/ManagedClientState.js index 1a26b0dab..10f71b4d2 100644 --- a/guacamole/src/main/webapp/app/client/types/ManagedClientState.js +++ b/guacamole/src/main/webapp/app/client/types/ManagedClientState.js @@ -45,6 +45,16 @@ angular.module('client').factory('ManagedClientState', [function defineManagedCl */ this.connectionState = template.connectionState || ManagedClientState.ConnectionState.IDLE; + /** + * Whether the network connection used by the tunnel seems unstable. If + * the network connection is unstable, the remote desktop connection + * may perform poorly or disconnect. + * + * @type Boolean + * @default false + */ + this.tunnelUnstable = template.tunnelUnstable || false; + /** * The status code of the current error condition, if connectionState * is CLIENT_ERROR or TUNNEL_ERROR. For all other connectionState @@ -93,15 +103,6 @@ angular.module('client').factory('ManagedClientState', [function defineManagedCl */ CONNECTED : "CONNECTED", - /** - * The Guacamole connection has been successfully established, but the - * network connection seems unstable. The connection may perform poorly - * or disconnect. - * - * @type String - */ - UNSTABLE : "UNSTABLE", - /** * The Guacamole connection has terminated successfully. No errors are * indicated. @@ -130,7 +131,9 @@ angular.module('client').factory('ManagedClientState', [function defineManagedCl /** * Sets the current client state and, if given, the associated status code. - * If an error is already represented, this function has no effect. + * If an error is already represented, this function has no effect. If the + * client state was previously marked as unstable, that flag is implicitly + * cleared. * * @param {ManagedClientState} clientState * The ManagedClientState to update. @@ -153,6 +156,7 @@ angular.module('client').factory('ManagedClientState', [function defineManagedCl // Update connection state clientState.connectionState = connectionState; + clientState.tunnelUnstable = false; // Set status code, if given if (statusCode) @@ -160,6 +164,22 @@ angular.module('client').factory('ManagedClientState', [function defineManagedCl }; + /** + * Updates the given client state, setting whether the underlying tunnel + * is currently unstable. An unstable tunnel is not necessarily + * disconnected, but appears to be misbehaving and may be disconnected. + * + * @param {ManagedClientState} clientState + * The ManagedClientState to update. + * + * @param {Boolean} unstable + * Whether the underlying tunnel of the connection currently appears + * unstable. + */ + ManagedClientState.setTunnelUnstable = function setTunnelUnstable(clientState, unstable) { + clientState.tunnelUnstable = unstable; + }; + return ManagedClientState; }]); \ No newline at end of file From 5825835237fbd3307f7da09b3958d4f458fe8598 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sat, 1 Sep 2018 21:12:10 -0700 Subject: [PATCH 2/5] GUACAMOLE-567: Add tunnel isConnected() function. Consider both OPEN and UNSTABLE status as connected. --- .../src/main/webapp/modules/Tunnel.js | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/guacamole-common-js/src/main/webapp/modules/Tunnel.js b/guacamole-common-js/src/main/webapp/modules/Tunnel.js index 52bd20ac2..06cc741a3 100644 --- a/guacamole-common-js/src/main/webapp/modules/Tunnel.js +++ b/guacamole-common-js/src/main/webapp/modules/Tunnel.js @@ -73,6 +73,17 @@ Guacamole.Tunnel = function() { }; + /** + * Returns whether this tunnel is currently connected. + * + * @returns {Boolean} + * true if this tunnel is currently connected, false otherwise. + */ + this.isConnected = function isConnected() { + return this.state === Guacamole.Tunnel.State.OPEN + || this.state === Guacamole.Tunnel.State.UNSTABLE; + }; + /** * The current state of this tunnel. * @@ -342,7 +353,7 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) { this.sendMessage = function() { // Do not attempt to send messages if not connected - if (tunnel.state !== Guacamole.Tunnel.State.OPEN) + if (!tunnel.isConnected()) return; // Do not attempt to send empty messages @@ -384,7 +395,7 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) { function sendPendingMessages() { // Do not attempt to send messages if not connected - if (tunnel.state !== Guacamole.Tunnel.State.OPEN) + if (!tunnel.isConnected()) return; if (outputMessageBuffer.length > 0) { @@ -462,7 +473,7 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) { function parseResponse() { // Do not handle responses if not connected - if (tunnel.state !== Guacamole.Tunnel.State.OPEN) { + if (!tunnel.isConnected()) { // Clean up interval if polling if (interval !== null) @@ -835,7 +846,7 @@ Guacamole.WebSocketTunnel = function(tunnelURL) { this.sendMessage = function(elements) { // Do not attempt to send messages if not connected - if (tunnel.state !== Guacamole.Tunnel.State.OPEN) + if (!tunnel.isConnected()) return; // Do not attempt to send empty messages @@ -945,7 +956,7 @@ Guacamole.WebSocketTunnel = function(tunnelURL) { var opcode = elements.shift(); // Update state and UUID when first instruction received - if (tunnel.state !== Guacamole.Tunnel.State.OPEN) { + if (tunnel.state === Guacamole.Tunnel.State.CONNECTING) { // Associate tunnel UUID if received if (opcode === Guacamole.Tunnel.INTERNAL_DATA_OPCODE) From ea0b33bee19123489684549f31c36356fe39c728 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 6 Sep 2018 19:48:33 -0700 Subject: [PATCH 3/5] GUACAMOLE-567: Use ping messages specific to the WebSocket tunnel to test connection stability independently of the underlying Guacamole connection. --- .../src/main/webapp/modules/Tunnel.js | 32 ++++- .../GuacamoleWebSocketTunnelEndpoint.java | 121 ++++++++++++++++-- 2 files changed, 141 insertions(+), 12 deletions(-) diff --git a/guacamole-common-js/src/main/webapp/modules/Tunnel.js b/guacamole-common-js/src/main/webapp/modules/Tunnel.js index 06cc741a3..827dd6cd4 100644 --- a/guacamole-common-js/src/main/webapp/modules/Tunnel.js +++ b/guacamole-common-js/src/main/webapp/modules/Tunnel.js @@ -153,7 +153,8 @@ Guacamole.Tunnel = function() { * use by tunnel implementations. The value of this opcode is guaranteed to be * the empty string (""). Tunnel implementations may use this opcode for any * purpose. It is currently used by the HTTP tunnel to mark the end of the HTTP - * response, and by the WebSocket tunnel to transmit the tunnel UUID. + * response, and by the WebSocket tunnel to transmit the tunnel UUID and send + * connection stability test pings/responses. * * @constant * @type {String} @@ -742,6 +743,15 @@ Guacamole.WebSocketTunnel = function(tunnelURL) { */ var unstableTimeout = null; + /** + * The current connection stability test ping interval ID, if any. This + * will only be set upon successful connection. + * + * @private + * @type {Number} + */ + var pingInterval = null; + /** * The WebSocket protocol corresponding to the protocol used for the current * location. @@ -752,6 +762,16 @@ Guacamole.WebSocketTunnel = function(tunnelURL) { "https:": "wss:" }; + /** + * The number of milliseconds to wait between connection stability test + * pings. + * + * @private + * @constant + * @type {Number} + */ + var PING_FREQUENCY = 500; + // Transform current URL to WebSocket URL // If not already a websocket URL @@ -828,6 +848,9 @@ Guacamole.WebSocketTunnel = function(tunnelURL) { window.clearTimeout(receive_timeout); window.clearTimeout(unstableTimeout); + // Cease connection test pings + window.clearInterval(pingInterval); + // Ignore if already closed if (tunnel.state === Guacamole.Tunnel.State.CLOSED) return; @@ -892,6 +915,13 @@ Guacamole.WebSocketTunnel = function(tunnelURL) { socket.onopen = function(event) { reset_timeout(); + + // Ping tunnel endpoint regularly to test connection stability + pingInterval = setInterval(function sendPing() { + tunnel.sendMessage(Guacamole.Tunnel.INTERNAL_DATA_OPCODE, + "ping", new Date().getTime()); + }, PING_FREQUENCY); + }; socket.onclose = function(event) { diff --git a/guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java b/guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java index 0e0262254..772ce64b2 100644 --- a/guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java +++ b/guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java @@ -20,6 +20,7 @@ package org.apache.guacamole.websocket; import java.io.IOException; +import java.util.List; import javax.websocket.CloseReason; import javax.websocket.CloseReason.CloseCode; import javax.websocket.Endpoint; @@ -36,6 +37,8 @@ import org.apache.guacamole.io.GuacamoleWriter; import org.apache.guacamole.net.GuacamoleTunnel; import org.apache.guacamole.GuacamoleClientException; import org.apache.guacamole.GuacamoleConnectionClosedException; +import org.apache.guacamole.protocol.FilteredGuacamoleWriter; +import org.apache.guacamole.protocol.GuacamoleFilter; import org.apache.guacamole.protocol.GuacamoleInstruction; import org.apache.guacamole.protocol.GuacamoleStatus; import org.slf4j.Logger; @@ -54,6 +57,15 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { */ private static final int BUFFER_SIZE = 8192; + /** + * The opcode of the instruction used to indicate a connection stability + * test ping request or response. Note that this instruction is + * encapsulated within an internal tunnel instruction (with the opcode + * being the empty string), thus this will actually be the value of the + * first element of the received instruction. + */ + private static final String PING_OPCODE = "ping"; + /** * Logger for this class. */ @@ -61,10 +73,17 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { /** * The underlying GuacamoleTunnel. WebSocket reads/writes will be handled - * as reads/writes to this tunnel. + * as reads/writes to this tunnel. This value may be null if no connection + * has been established. */ private GuacamoleTunnel tunnel; - + + /** + * Remote (client) side of this connection. This value will always be + * non-null if tunnel is non-null. + */ + private RemoteEndpoint.Basic remote; + /** * Sends the numeric Guacaomle Status Code and Web Socket * code and closes the connection. @@ -107,6 +126,52 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { guacStatus.getWebSocketCode()); } + /** + * Sends a Guacamole instruction along the outbound WebSocket connection to + * the connected Guacamole client. If an instruction is already in the + * process of being sent by another thread, this function will block until + * in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from being + * sent. + */ + private void sendInstruction(String instruction) + throws IOException { + + // NOTE: Synchronization on the non-final remote field here is + // intentional. The remote (the outbound websocket connection) is only + // sensitive to simultaneous attempts to send messages with respect to + // itself. If the remote changes, then the outbound websocket + // connection has changed, and synchronization need only be performed + // in context of the new remote. + synchronized (remote) { + remote.sendText(instruction); + } + + } + + /** + * Sends a Guacamole instruction along the outbound WebSocket connection to + * the connected Guacamole client. If an instruction is already in the + * process of being sent by another thread, this function will block until + * in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from being + * sent. + */ + private void sendInstruction(GuacamoleInstruction instruction) + throws IOException { + sendInstruction(instruction.toString()); + } + /** * Returns a new tunnel for the given session. How this tunnel is created * or retrieved is implementation-dependent. @@ -126,6 +191,9 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { @OnOpen public void onOpen(final Session session, EndpointConfig config) { + // Store underlying remote for future use via sendInstruction() + remote = session.getBasicRemote(); + try { // Get tunnel @@ -157,11 +225,6 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { // Prepare read transfer thread Thread readThread = new Thread() { - /** - * Remote (client) side of this connection - */ - private final RemoteEndpoint.Basic remote = session.getBasicRemote(); - @Override public void run() { @@ -172,10 +235,10 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { try { // Send tunnel UUID - remote.sendText(new GuacamoleInstruction( + sendInstruction(new GuacamoleInstruction( GuacamoleTunnel.INTERNAL_DATA_OPCODE, tunnel.getUUID().toString() - ).toString()); + )); try { @@ -187,7 +250,7 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { // Flush if we expect to wait or buffer is getting full if (!reader.available() || buffer.length() >= BUFFER_SIZE) { - remote.sendText(buffer.toString()); + sendInstruction(buffer.toString()); buffer.setLength(0); } @@ -239,7 +302,43 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { if (tunnel == null) return; - GuacamoleWriter writer = tunnel.acquireWriter(); + // Filter received instructions, handling tunnel-internal instructions + // without passing through to guacd + GuacamoleWriter writer = new FilteredGuacamoleWriter(tunnel.acquireWriter(), new GuacamoleFilter() { + + @Override + public GuacamoleInstruction filter(GuacamoleInstruction instruction) + throws GuacamoleException { + + // Filter out all tunnel-internal instructions + if (instruction.getOpcode().equals(GuacamoleTunnel.INTERNAL_DATA_OPCODE)) { + + // Respond to ping requests + List args = instruction.getArgs(); + if (args.size() >= 2 && args.get(0).equals(PING_OPCODE)) { + + try { + sendInstruction(new GuacamoleInstruction( + GuacamoleTunnel.INTERNAL_DATA_OPCODE, + PING_OPCODE, args.get(1) + )); + } + catch (IOException e) { + logger.debug("Unable to send \"ping\" response for WebSocket tunnel.", e); + } + + } + + return null; + + } + + // Pass through all non-internal instructions untouched + return instruction; + + } + + }); try { // Write received message From 819d3178343a84af6926d862dc9cf584f9aa80f1 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 6 Sep 2018 19:49:02 -0700 Subject: [PATCH 4/5] GUACAMOLE-567: Add support for WebSocket-specific ping messages to the legacy WebSocket tunnel implementations. --- .../GuacamoleWebSocketTunnelServlet.java | 111 +++++++++++++++- .../GuacamoleWebSocketTunnelListener.java | 121 ++++++++++++++++-- .../GuacamoleWebSocketTunnelServlet.java | 110 +++++++++++++++- 3 files changed, 323 insertions(+), 19 deletions(-) diff --git a/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java b/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java index e5c5db9a1..304e1dd95 100644 --- a/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java +++ b/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java @@ -20,6 +20,7 @@ package org.apache.guacamole.tunnel.websocket.jetty8; import java.io.IOException; +import java.util.List; import javax.servlet.http.HttpServletRequest; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.io.GuacamoleReader; @@ -30,6 +31,8 @@ import org.eclipse.jetty.websocket.WebSocket.Connection; import org.eclipse.jetty.websocket.WebSocketServlet; import org.apache.guacamole.GuacamoleClientException; import org.apache.guacamole.GuacamoleConnectionClosedException; +import org.apache.guacamole.protocol.FilteredGuacamoleWriter; +import org.apache.guacamole.protocol.GuacamoleFilter; import org.apache.guacamole.protocol.GuacamoleInstruction; import org.apache.guacamole.tunnel.http.HTTPTunnelRequest; import org.apache.guacamole.tunnel.TunnelRequest; @@ -52,6 +55,15 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { */ private static final int BUFFER_SIZE = 8192; + /** + * The opcode of the instruction used to indicate a connection stability + * test ping request or response. Note that this instruction is + * encapsulated within an internal tunnel instruction (with the opcode + * being the empty string), thus this will actually be the value of the + * first element of the received instruction. + */ + private static final String PING_OPCODE = "ping"; + /** * Sends the given numeric Guacamole and WebSocket status * on the given WebSocket connection and closes the @@ -106,6 +118,58 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { */ private GuacamoleTunnel tunnel = null; + /** + * The active WebSocket connection. This value will always be + * non-null if tunnel is non-null. + */ + private Connection connection = null; + + /** + * Sends a Guacamole instruction along the outbound WebSocket + * connection to the connected Guacamole client. If an instruction + * is already in the process of being sent by another thread, this + * function will block until in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from + * being sent. + */ + private void sendInstruction(String instruction) + throws IOException { + + // NOTE: Synchronization on the non-final remote field here is + // intentional. The outbound websocket connection is only + // sensitive to simultaneous attempts to send messages with + // respect to itself. If the connection changes, then + // synchronization need only be performed in context of the new + // connection + synchronized (connection) { + connection.sendMessage(instruction); + } + + } + + /** + * Sends a Guacamole instruction along the outbound WebSocket + * connection to the connected Guacamole client. If an instruction + * is already in the process of being sent by another thread, this + * function will block until in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from being + * sent. + */ + private void sendInstruction(GuacamoleInstruction instruction) + throws IOException { + sendInstruction(instruction.toString()); + } + @Override public void onMessage(String string) { @@ -113,7 +177,43 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { if (tunnel == null) return; - GuacamoleWriter writer = tunnel.acquireWriter(); + // Filter received instructions, handling tunnel-internal + // instructions without passing through to guacd + GuacamoleWriter writer = new FilteredGuacamoleWriter(tunnel.acquireWriter(), new GuacamoleFilter() { + + @Override + public GuacamoleInstruction filter(GuacamoleInstruction instruction) + throws GuacamoleException { + + // Filter out all tunnel-internal instructions + if (instruction.getOpcode().equals(GuacamoleTunnel.INTERNAL_DATA_OPCODE)) { + + // Respond to ping requests + List args = instruction.getArgs(); + if (args.size() >= 2 && args.get(0).equals(PING_OPCODE)) { + + try { + sendInstruction(new GuacamoleInstruction( + GuacamoleTunnel.INTERNAL_DATA_OPCODE, + PING_OPCODE, args.get(1) + )); + } + catch (IOException e) { + logger.debug("Unable to send \"ping\" response for WebSocket tunnel.", e); + } + + } + + return null; + + } + + // Pass through all non-internal instructions untouched + return instruction; + + } + + }); // Write message received try { @@ -133,6 +233,9 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { @Override public void onOpen(final Connection connection) { + // Store websocket connection for future use via sendInstruction() + this.connection = connection; + try { tunnel = doConnect(tunnelRequest); } @@ -162,10 +265,10 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { try { // Send tunnel UUID - connection.sendMessage(new GuacamoleInstruction( + sendInstruction(new GuacamoleInstruction( GuacamoleTunnel.INTERNAL_DATA_OPCODE, tunnel.getUUID().toString() - ).toString()); + )); try { @@ -177,7 +280,7 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { // Flush if we expect to wait or buffer is getting full if (!reader.available() || buffer.length() >= BUFFER_SIZE) { - connection.sendMessage(buffer.toString()); + sendInstruction(buffer.toString()); buffer.setLength(0); } diff --git a/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty9/GuacamoleWebSocketTunnelListener.java b/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty9/GuacamoleWebSocketTunnelListener.java index 0594d06aa..6422f5718 100644 --- a/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty9/GuacamoleWebSocketTunnelListener.java +++ b/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/jetty9/GuacamoleWebSocketTunnelListener.java @@ -20,6 +20,7 @@ package org.apache.guacamole.tunnel.websocket.jetty9; import java.io.IOException; +import java.util.List; import org.eclipse.jetty.websocket.api.CloseStatus; import org.eclipse.jetty.websocket.api.RemoteEndpoint; import org.eclipse.jetty.websocket.api.Session; @@ -30,6 +31,8 @@ import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.io.GuacamoleReader; import org.apache.guacamole.io.GuacamoleWriter; import org.apache.guacamole.net.GuacamoleTunnel; +import org.apache.guacamole.protocol.FilteredGuacamoleWriter; +import org.apache.guacamole.protocol.GuacamoleFilter; import org.apache.guacamole.protocol.GuacamoleInstruction; import org.apache.guacamole.protocol.GuacamoleStatus; import org.slf4j.Logger; @@ -45,6 +48,15 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe */ private static final int BUFFER_SIZE = 8192; + /** + * The opcode of the instruction used to indicate a connection stability + * test ping request or response. Note that this instruction is + * encapsulated within an internal tunnel instruction (with the opcode + * being the empty string), thus this will actually be the value of the + * first element of the received instruction. + */ + private static final String PING_OPCODE = "ping"; + /** * Logger for this class. */ @@ -52,10 +64,17 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe /** * The underlying GuacamoleTunnel. WebSocket reads/writes will be handled - * as reads/writes to this tunnel. + * as reads/writes to this tunnel. This value may be null if no connection + * has been established. */ private GuacamoleTunnel tunnel; - + + /** + * Remote (client) side of this connection. This value will always be + * non-null if tunnel is non-null. + */ + private RemoteEndpoint remote; + /** * Sends the given numeric Guacamole and WebSocket status * codes on the given WebSocket connection and closes the @@ -101,6 +120,52 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe } + /** + * Sends a Guacamole instruction along the outbound WebSocket connection to + * the connected Guacamole client. If an instruction is already in the + * process of being sent by another thread, this function will block until + * in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from being + * sent. + */ + private void sendInstruction(String instruction) + throws IOException { + + // NOTE: Synchronization on the non-final remote field here is + // intentional. The remote (the outbound websocket connection) is only + // sensitive to simultaneous attempts to send messages with respect to + // itself. If the remote changes, then the outbound websocket + // connection has changed, and synchronization need only be performed + // in context of the new remote. + synchronized (remote) { + remote.sendString(instruction); + } + + } + + /** + * Sends a Guacamole instruction along the outbound WebSocket connection to + * the connected Guacamole client. If an instruction is already in the + * process of being sent by another thread, this function will block until + * in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from being + * sent. + */ + private void sendInstruction(GuacamoleInstruction instruction) + throws IOException { + sendInstruction(instruction.toString()); + } + /** * Returns a new tunnel for the given session. How this tunnel is created * or retrieved is implementation-dependent. @@ -117,6 +182,9 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe @Override public void onWebSocketConnect(final Session session) { + // Store underlying remote for future use via sendInstruction() + remote = session.getRemote(); + try { // Get tunnel @@ -137,11 +205,6 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe // Prepare read transfer thread Thread readThread = new Thread() { - /** - * Remote (client) side of this connection - */ - private final RemoteEndpoint remote = session.getRemote(); - @Override public void run() { @@ -152,10 +215,10 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe try { // Send tunnel UUID - remote.sendString(new GuacamoleInstruction( + sendInstruction(new GuacamoleInstruction( GuacamoleTunnel.INTERNAL_DATA_OPCODE, tunnel.getUUID().toString() - ).toString()); + )); try { @@ -167,7 +230,7 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe // Flush if we expect to wait or buffer is getting full if (!reader.available() || buffer.length() >= BUFFER_SIZE) { - remote.sendString(buffer.toString()); + sendInstruction(buffer.toString()); buffer.setLength(0); } @@ -219,7 +282,43 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe if (tunnel == null) return; - GuacamoleWriter writer = tunnel.acquireWriter(); + // Filter received instructions, handling tunnel-internal instructions + // without passing through to guacd + GuacamoleWriter writer = new FilteredGuacamoleWriter(tunnel.acquireWriter(), new GuacamoleFilter() { + + @Override + public GuacamoleInstruction filter(GuacamoleInstruction instruction) + throws GuacamoleException { + + // Filter out all tunnel-internal instructions + if (instruction.getOpcode().equals(GuacamoleTunnel.INTERNAL_DATA_OPCODE)) { + + // Respond to ping requests + List args = instruction.getArgs(); + if (args.size() >= 2 && args.get(0).equals(PING_OPCODE)) { + + try { + sendInstruction(new GuacamoleInstruction( + GuacamoleTunnel.INTERNAL_DATA_OPCODE, + PING_OPCODE, args.get(1) + )); + } + catch (IOException e) { + logger.debug("Unable to send \"ping\" response for WebSocket tunnel.", e); + } + + } + + return null; + + } + + // Pass through all non-internal instructions untouched + return instruction; + + } + + }); try { // Write received message diff --git a/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java b/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java index a2e8b3981..215cc8f05 100644 --- a/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java +++ b/guacamole/src/main/java/org/apache/guacamole/tunnel/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java @@ -35,6 +35,8 @@ import org.apache.catalina.websocket.WebSocketServlet; import org.apache.catalina.websocket.WsOutbound; import org.apache.guacamole.GuacamoleClientException; import org.apache.guacamole.GuacamoleConnectionClosedException; +import org.apache.guacamole.protocol.FilteredGuacamoleWriter; +import org.apache.guacamole.protocol.GuacamoleFilter; import org.apache.guacamole.protocol.GuacamoleInstruction; import org.apache.guacamole.tunnel.http.HTTPTunnelRequest; import org.apache.guacamole.tunnel.TunnelRequest; @@ -52,6 +54,15 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { */ private static final int BUFFER_SIZE = 8192; + /** + * The opcode of the instruction used to indicate a connection stability + * test ping request or response. Note that this instruction is + * encapsulated within an internal tunnel instruction (with the opcode + * being the empty string), thus this will actually be the value of the + * first element of the received instruction. + */ + private static final String PING_OPCODE = "ping"; + /** * Logger for this class. */ @@ -130,6 +141,58 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { */ private GuacamoleTunnel tunnel = null; + /** + * The outbound half of the WebSocket connection. This value will + * always be non-null if tunnel is non-null. + */ + private WsOutbound outbound = null; + + /** + * Sends a Guacamole instruction along the outbound WebSocket + * connection to the connected Guacamole client. If an instruction + * is already in the process of being sent by another thread, this + * function will block until in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from + * being sent. + */ + private void sendInstruction(CharSequence instruction) + throws IOException { + + // NOTE: Synchronization on the non-final remote field here is + // intentional. The outbound websocket connection is only + // sensitive to simultaneous attempts to send messages with + // respect to itself. If the connection changes, then + // synchronization need only be performed in context of the new + // connection + synchronized (outbound) { + outbound.writeTextMessage(CharBuffer.wrap(instruction)); + } + + } + + /** + * Sends a Guacamole instruction along the outbound WebSocket + * connection to the connected Guacamole client. If an instruction + * is already in the process of being sent by another thread, this + * function will block until in-progress instructions are complete. + * + * @param instruction + * The instruction to send. + * + * @throws IOException + * If an I/O error occurs preventing the given instruction from being + * sent. + */ + private void sendInstruction(GuacamoleInstruction instruction) + throws IOException { + sendInstruction(instruction.toString()); + } + @Override protected void onTextData(Reader reader) throws IOException { @@ -137,7 +200,43 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { if (tunnel == null) return; - GuacamoleWriter writer = tunnel.acquireWriter(); + // Filter received instructions, handling tunnel-internal + // instructions without passing through to guacd + GuacamoleWriter writer = new FilteredGuacamoleWriter(tunnel.acquireWriter(), new GuacamoleFilter() { + + @Override + public GuacamoleInstruction filter(GuacamoleInstruction instruction) + throws GuacamoleException { + + // Filter out all tunnel-internal instructions + if (instruction.getOpcode().equals(GuacamoleTunnel.INTERNAL_DATA_OPCODE)) { + + // Respond to ping requests + List args = instruction.getArgs(); + if (args.size() >= 2 && args.get(0).equals(PING_OPCODE)) { + + try { + sendInstruction(new GuacamoleInstruction( + GuacamoleTunnel.INTERNAL_DATA_OPCODE, + PING_OPCODE, args.get(1) + )); + } + catch (IOException e) { + logger.debug("Unable to send \"ping\" response for WebSocket tunnel.", e); + } + + } + + return null; + + } + + // Pass through all non-internal instructions untouched + return instruction; + + } + + }); // Write all available data try { @@ -162,6 +261,9 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { @Override public void onOpen(final WsOutbound outbound) { + // Store outbound connection for future use via sendInstruction() + this.outbound = outbound; + try { tunnel = doConnect(tunnelRequest); } @@ -191,10 +293,10 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { try { // Send tunnel UUID - outbound.writeTextMessage(CharBuffer.wrap(new GuacamoleInstruction( + sendInstruction(new GuacamoleInstruction( GuacamoleTunnel.INTERNAL_DATA_OPCODE, tunnel.getUUID().toString() - ).toString())); + )); try { @@ -206,7 +308,7 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { // Flush if we expect to wait or buffer is getting full if (!reader.available() || buffer.length() >= BUFFER_SIZE) { - outbound.writeTextMessage(CharBuffer.wrap(buffer)); + sendInstruction(CharBuffer.wrap(buffer)); buffer.setLength(0); } From 34bab9524eb6d1b5e3fa96be7ed2602d71f25dd9 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 6 Sep 2018 19:57:29 -0700 Subject: [PATCH 5/5] GUACAMOLE-567: Regularly test connection stability of HTTP tunnel. Unlike the WebSocket tunnel, where a manual ping request/response must be explicitly implemented, we can rely on HTTP's own request/response to verify stability. --- .../src/main/webapp/modules/Tunnel.js | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/guacamole-common-js/src/main/webapp/modules/Tunnel.js b/guacamole-common-js/src/main/webapp/modules/Tunnel.js index 827dd6cd4..24ffaa860 100644 --- a/guacamole-common-js/src/main/webapp/modules/Tunnel.js +++ b/guacamole-common-js/src/main/webapp/modules/Tunnel.js @@ -258,6 +258,25 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) { */ var unstableTimeout = null; + /** + * The current connection stability test ping interval ID, if any. This + * will only be set upon successful connection. + * + * @private + * @type {Number} + */ + var pingInterval = null; + + /** + * The number of milliseconds to wait between connection stability test + * pings. + * + * @private + * @constant + * @type {Number} + */ + var PING_FREQUENCY = 500; + /** * Additional headers to be sent in tunnel requests. This dictionary can be * populated with key/value header pairs to pass information such as authentication @@ -327,6 +346,9 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) { window.clearTimeout(receive_timeout); window.clearTimeout(unstableTimeout); + // Cease connection test pings + window.clearInterval(pingInterval); + // Ignore if already closed if (tunnel.state === Guacamole.Tunnel.State.CLOSED) return; @@ -413,6 +435,8 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) { message_xmlhttprequest.onreadystatechange = function() { if (message_xmlhttprequest.readyState === 4) { + reset_timeout(); + // If an error occurs during send, handle it if (message_xmlhttprequest.status !== 200) handleHTTPTunnelError(message_xmlhttprequest); @@ -687,6 +711,11 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) { // Mark as open tunnel.setState(Guacamole.Tunnel.State.OPEN); + // Ping tunnel endpoint regularly to test connection stability + pingInterval = setInterval(function sendPing() { + tunnel.sendMessage("nop"); + }, PING_FREQUENCY); + // Start reading data handleResponse(makeRequest());