From 002ec0c50e7455f9ee5be2c450221bd0800740b4 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 16 Dec 2015 17:50:18 -0800 Subject: [PATCH] GUAC-1427: Favor ConcurrentHashMap over Collections.synchronizedMap(). Keeping sessions/tunnels in order is not worth the extreme overhead of a map-wide lock. --- .../servlet/GuacamoleHTTPTunnelMap.java | 20 ++++++------- .../basic/rest/auth/BasicTokenSessionMap.java | 28 ++++++++----------- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/guacamole-common/src/main/java/org/glyptodon/guacamole/servlet/GuacamoleHTTPTunnelMap.java b/guacamole-common/src/main/java/org/glyptodon/guacamole/servlet/GuacamoleHTTPTunnelMap.java index d7f53ce2b..184d93303 100644 --- a/guacamole-common/src/main/java/org/glyptodon/guacamole/servlet/GuacamoleHTTPTunnelMap.java +++ b/guacamole-common/src/main/java/org/glyptodon/guacamole/servlet/GuacamoleHTTPTunnelMap.java @@ -22,10 +22,10 @@ package org.glyptodon.guacamole.servlet; -import java.util.Collections; import java.util.Iterator; -import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -65,8 +65,8 @@ class GuacamoleHTTPTunnelMap { /** * Map of all tunnels that are using HTTP, indexed by tunnel UUID. */ - private final Map tunnelMap = - Collections.synchronizedMap(new LinkedHashMap(16, 0.75f, true)); + private final ConcurrentMap tunnelMap = + new ConcurrentHashMap(); /** * Creates a new GuacamoleHTTPTunnelMap which automatically closes and @@ -124,9 +124,12 @@ class GuacamoleHTTPTunnelMap { // If tunnel is too old, close and remove it if (age >= tunnelTimeout) { + + // Remove old entry logger.debug("HTTP tunnel \"{}\" has timed out.", entry.getKey()); entries.remove(); + // Attempt to close tunnel try { tunnel.close(); } @@ -136,14 +139,9 @@ class GuacamoleHTTPTunnelMap { } - // Otherwise, this tunnel has been recently used, as have all - // other tunnels following this one within tunnelMap - else - break; + } // end for each tunnel - } - - } + } // end timeout task run() } diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/rest/auth/BasicTokenSessionMap.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/rest/auth/BasicTokenSessionMap.java index bbeaa43db..fcc410985 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/rest/auth/BasicTokenSessionMap.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/rest/auth/BasicTokenSessionMap.java @@ -22,10 +22,10 @@ package org.glyptodon.guacamole.net.basic.rest.auth; -import java.util.Collections; import java.util.Iterator; -import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -57,8 +57,8 @@ public class BasicTokenSessionMap implements TokenSessionMap { /** * Keeps track of the authToken to GuacamoleSession mapping. */ - private final Map sessionMap = - Collections.synchronizedMap(new LinkedHashMap(16, 0.75f, true)); + private final ConcurrentMap sessionMap = + new ConcurrentHashMap(); /** * Create a new BasicTokenGuacamoleSessionMap configured using the given @@ -89,9 +89,7 @@ public class BasicTokenSessionMap implements TokenSessionMap { /** * Task which iterates through all active sessions, evicting those sessions - * which are beyond the session timeout. This is a fairly easy thing to do, - * since the session storage structure guarantees that sessions are always - * in descending order of age. + * which are beyond the session timeout. */ private class SessionEvictionTask implements Runnable { @@ -114,11 +112,11 @@ public class BasicTokenSessionMap implements TokenSessionMap { @Override public void run() { - // Get current time - long now = System.currentTimeMillis(); + // Get start time of session check time + long sessionCheckStart = System.currentTimeMillis(); logger.debug("Checking for expired sessions..."); - + // For each session, remove sesions which have expired Iterator> entries = sessionMap.entrySet().iterator(); while (entries.hasNext()) { @@ -131,7 +129,7 @@ public class BasicTokenSessionMap implements TokenSessionMap { continue; // Get elapsed time since last access - long age = now - session.getLastAccessedTime(); + long age = sessionCheckStart - session.getLastAccessedTime(); // If session is too old, evict it and check the next one if (age >= sessionTimeout) { @@ -140,13 +138,11 @@ public class BasicTokenSessionMap implements TokenSessionMap { session.invalidate(); } - // Otherwise, no other sessions can possibly be old enough - else - break; - } - logger.debug("Session check complete."); + // Log completion and duration + logger.debug("Session check completed in {} ms.", + System.currentTimeMillis() - sessionCheckStart); }