diff --git a/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java b/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java index e51ea76cf..79d757bdd 100644 --- a/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java +++ b/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java @@ -135,7 +135,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { return authProvider.getResource(); } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -153,7 +153,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { return authProvider.updateCredentials(credentials); } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -249,7 +249,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -272,7 +272,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { return authProvider.updateAuthenticatedUser(authenticatedUser, credentials); } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -318,7 +318,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -342,7 +342,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { return authProvider.updateUserContext(context, authenticatedUser, credentials); } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -362,7 +362,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { return authProvider.decorate(context, authenticatedUser, credentials); } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -383,7 +383,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { authenticatedUser, credentials); } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -396,7 +396,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { authProvider.shutdown(); } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } diff --git a/guacamole/src/main/java/org/apache/guacamole/log/InheritableMDCAdapter.java b/guacamole/src/main/java/org/apache/guacamole/log/InheritableMDCAdapter.java index 493925476..e75586b47 100644 --- a/guacamole/src/main/java/org/apache/guacamole/log/InheritableMDCAdapter.java +++ b/guacamole/src/main/java/org/apache/guacamole/log/InheritableMDCAdapter.java @@ -33,8 +33,26 @@ import org.slf4j.spi.MDCAdapter; */ public class InheritableMDCAdapter implements MDCAdapter { + // + // IMPORTANT: Thread-local values MUST be removed at web application + // shutdown or a memory leak may result. To avoid this, care must be + // taken in this implementation: + // + // * Ensure that a balanced remove() for every put() is sufficient to + // clear and remove the thread-local itself. + // + // * Calls to ThreadLocal.get() implicitly assign a value to the + // thread-local if no value was present before. This value needs to be + // removed to avoid leaking references at web application shutdown. + // + /** * MDC context map that is automatically inherited by child threads. + *

+ * IMPORTANT: As a thread-local value, it is critical that this value be + * removed at web application shutdown or a memory leak may result. Here, + * we maintain a reference within the thread-local only as long as at least + * one value is stored. Once the map is empty, the thread-local is removed. */ private final InheritableThreadLocal> context = new InheritableThreadLocal>() { @@ -64,6 +82,12 @@ public class InheritableMDCAdapter implements MDCAdapter { * {@link #get(java.lang.String)} which are intentionally stored * separately in {@link #context}. This matches the behavior of the Logback * implementation of MDC. + *

+ * IMPORTANT: As a thread-local value, it is critical that this value be + * removed at web application shutdown or a memory leak may result. Here, + * we maintain a reference within the thread-local only as long as at least + * one non-empty deque is stored. Once no non-empty deques are present + * empty, the thread-local is removed. */ private final ThreadLocal>> contextDeques = new ThreadLocal>>() { @@ -82,30 +106,67 @@ public class InheritableMDCAdapter implements MDCAdapter { @Override public String get(String key) { - return context.get().get(key); + + ForkableHashMap contextMap = context.get(); + + // Do not maintain a thread-local reference to an empty map + if (contextMap.isEmpty()) { + context.remove(); + return null; + } + + return contextMap.get(key); + } @Override public void remove(String key) { - context.get().remove(key); + + ForkableHashMap contextMap = context.get(); + contextMap.remove(key); + + // Do not maintain a thread-local reference to an empty map + if (contextMap.isEmpty()) + context.remove(); + } @Override public void clear() { - context.get().clear(); + // Do not maintain a thread-local reference to an empty map + context.remove(); } @Override public Map getCopyOfContextMap() { - // This function is invoked by Logback for every log message produced, - // so simply copying the context map is expensive. We instead create a - // copy-on-write reference to the context map. - return context.get().fork(); + + ForkableHashMap contextMap = context.get(); + + // Do not maintain a thread-local reference to an empty map - instead + // repurpose the new empty map as the "copy" + if (contextMap.isEmpty()) { + context.remove(); + return contextMap; + } + + // Universally copying the context map would be expensive, as the + // getCopyOfContextMap() function is invoked by Logback for every log + // message produced. Instead, we create a copy-on-write reference to + // the context map. + return contextMap.fork(); + } @Override public void setContextMap(Map contextMap) { - context.set(new ForkableHashMap<>(contextMap)); + + // Do not maintain a thread-local reference to an empty map + if (contextMap.isEmpty()) + context.remove(); + + else + context.set(new ForkableHashMap<>(contextMap)); + } /** @@ -139,17 +200,46 @@ public class InheritableMDCAdapter implements MDCAdapter { @Override public String popByKey(String key) { - return getDeque(key).pop(); + + Deque deque = getDeque(key); + String value = deque.pop(); + + // Do not maintain any references to empty deques (so that it's easy to + // detect when there are no items in any deques) + if (deque.isEmpty()) + clearDequeByKey(key); + + return value; + } @Override public Deque getCopyOfDequeByKey(String key) { - return new LinkedList<>(getDeque(key)); + + Deque deque = getDeque(key); + + // Do not maintain a thread-local reference to an empty deque - instead + // repurpose the new empty deque as the "copy" + if (deque.isEmpty()) { + clearDequeByKey(key); // ... and ensure the deque map is removed if empty, too + return deque; + } + + return new LinkedList<>(deque); + } @Override public void clearDequeByKey(String key) { - getDeque(key).clear(); + + // Clear by entirely removing + Map> dequeMap = contextDeques.get(); + dequeMap.remove(key); + + // Do not maintain a thread-local reference to an empty map + if (dequeMap.isEmpty()) + contextDeques.remove(); + } } diff --git a/guacamole/src/main/java/org/apache/guacamole/rest/session/SessionResource.java b/guacamole/src/main/java/org/apache/guacamole/rest/session/SessionResource.java index 9e9707e10..7d69d180e 100644 --- a/guacamole/src/main/java/org/apache/guacamole/rest/session/SessionResource.java +++ b/guacamole/src/main/java/org/apache/guacamole/rest/session/SessionResource.java @@ -126,7 +126,7 @@ public class SessionResource { } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } } @@ -168,7 +168,7 @@ public class SessionResource { } finally { - MDC.put(LogModule.MDC_CONTEXT_KEY, LogModule.MDC_CONTEXT_ROOT); + MDC.remove(LogModule.MDC_CONTEXT_KEY); } }