Repository: tomee Updated Branches: refs/heads/master fc14d71df -> c38893cb9
TOMEE-1857 avoid Logger.getInstance to leak Project: http://git-wip-us.apache.org/repos/asf/tomee/repo Commit: http://git-wip-us.apache.org/repos/asf/tomee/commit/c38893cb Tree: http://git-wip-us.apache.org/repos/asf/tomee/tree/c38893cb Diff: http://git-wip-us.apache.org/repos/asf/tomee/diff/c38893cb Branch: refs/heads/master Commit: c38893cb954b9c5ad7af030025d7951505f759a7 Parents: fc14d71 Author: Romain manni-Bucau <rmannibu...@gmail.com> Authored: Mon Jul 4 08:42:29 2016 +0200 Committer: Romain manni-Bucau <rmannibu...@gmail.com> Committed: Mon Jul 4 08:42:29 2016 +0200 ---------------------------------------------------------------------- .../org/apache/openejb/util/LogCategory.java | 11 +++- .../java/org/apache/openejb/util/Logger.java | 52 ++++++++++++++---- .../java/org/apache/openejb/util/Memoizer.java | 4 ++ .../org/apache/openejb/util/MemoizerTest.java | 55 ++++++++++++++++++++ 4 files changed, 110 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tomee/blob/c38893cb/container/openejb-core/src/main/java/org/apache/openejb/util/LogCategory.java ---------------------------------------------------------------------- diff --git a/container/openejb-core/src/main/java/org/apache/openejb/util/LogCategory.java b/container/openejb-core/src/main/java/org/apache/openejb/util/LogCategory.java index 49b3ffd..e1491cd 100644 --- a/container/openejb-core/src/main/java/org/apache/openejb/util/LogCategory.java +++ b/container/openejb-core/src/main/java/org/apache/openejb/util/LogCategory.java @@ -61,7 +61,7 @@ public final class LogCategory { public static final LogCategory OPENEJB_SQL = OPENEJB.createChild("sql"); private LogCategory(final String name) { - this.name = name; + this.name = name == null ? "" : name; } public String getName() { @@ -80,4 +80,13 @@ public final class LogCategory { return new LogCategory(this.name + "." + child); } + @Override + public boolean equals(final Object o) { + return this == o || !(o == null || getClass() != o.getClass()) && name.equals(LogCategory.class.cast(o).name); + } + + @Override + public int hashCode() { + return name.hashCode(); + } } http://git-wip-us.apache.org/repos/asf/tomee/blob/c38893cb/container/openejb-core/src/main/java/org/apache/openejb/util/Logger.java ---------------------------------------------------------------------- diff --git a/container/openejb-core/src/main/java/org/apache/openejb/util/Logger.java b/container/openejb-core/src/main/java/org/apache/openejb/util/Logger.java index b32026b..605db83 100644 --- a/container/openejb-core/src/main/java/org/apache/openejb/util/Logger.java +++ b/container/openejb-core/src/main/java/org/apache/openejb/util/Logger.java @@ -22,6 +22,7 @@ import org.apache.openejb.loader.SystemInstance; import java.io.File; import java.io.IOException; +import java.io.Serializable; import java.text.MessageFormat; import java.util.ArrayList; import java.util.List; @@ -231,13 +232,10 @@ public class Logger { /** * Builds a Logger object and returns it */ - private static final Computable<Object[], Logger> loggerResolver = new Computable<Object[], Logger>() { + private static final Computable<LoggerKey, Logger> loggerResolver = new Computable<LoggerKey, Logger>() { @Override - public Logger compute(final Object[] args) throws InterruptedException { - final LogCategory category = (LogCategory) args[0]; - final LogStream logStream = logStreamFactory.createLogStream(category); - final String baseName = (String) args[1]; - return new Logger(category, logStream, baseName); + public Logger compute(final LoggerKey args) throws InterruptedException { + return new Logger(args.category, logStreamFactory.createLogStream(args.category), args.baseName); } }; @@ -254,22 +252,22 @@ public class Logger { /** * Cache of parent-child relationships between resource names */ - private static final Computable<String, String> heirarchyCache = new Memoizer<String, String>(heirarchyResolver); + private static final Computable<String, String> heirarchyCache = new Memoizer<>(heirarchyResolver); /** * Cache of ResourceBundles */ - private static final Computable<String, ResourceBundle> bundleCache = new Memoizer<String, ResourceBundle>(bundleResolver); + private static final Computable<String, ResourceBundle> bundleCache = new Memoizer<>(bundleResolver); /** * Cache of Loggers */ - private static final Computable<Object[], Logger> loggerCache = new Memoizer<Object[], Logger>(loggerResolver); + private static final Computable<LoggerKey, Logger> loggerCache = new Memoizer<>(loggerResolver); /** * Cache of MessageFormats */ - private static final Computable<String, MessageFormat> messageFormatCache = new Memoizer<String, MessageFormat>(messageFormatResolver); + private static final Computable<String, MessageFormat> messageFormatCache = new Memoizer<>(messageFormatResolver); /** * Finds a Logger from the cache and returns it. If not found in cache then builds a Logger and returns it. @@ -282,7 +280,7 @@ public class Logger { configure(); try { - return loggerCache.compute(new Object[]{category, baseName}); + return loggerCache.compute(new LoggerKey(category, baseName)); } catch (final InterruptedException e) { // Don't return null here. Just create a new Logger and set it up. // It will not be stored in the cache, but a later lookup for the @@ -697,4 +695,36 @@ public class Logger { return key; } + protected static class LoggerKey implements Serializable { + protected final LogCategory category; + protected final String baseName; + private final int hash; + + protected LoggerKey(final LogCategory category, final String baseName) { + this.category = category; + this.baseName = baseName; + + int result = category.hashCode(); + hash = 31 * result + baseName.hashCode(); + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + final LoggerKey loggerKey = LoggerKey.class.cast(o); + return category.equals(loggerKey.category) && baseName.equals(loggerKey.baseName); + + } + + @Override + public int hashCode() { + return hash; + } + } } http://git-wip-us.apache.org/repos/asf/tomee/blob/c38893cb/container/openejb-core/src/main/java/org/apache/openejb/util/Memoizer.java ---------------------------------------------------------------------- diff --git a/container/openejb-core/src/main/java/org/apache/openejb/util/Memoizer.java b/container/openejb-core/src/main/java/org/apache/openejb/util/Memoizer.java index a94df8c..7b1c035 100644 --- a/container/openejb-core/src/main/java/org/apache/openejb/util/Memoizer.java +++ b/container/openejb-core/src/main/java/org/apache/openejb/util/Memoizer.java @@ -69,4 +69,8 @@ public class Memoizer<K, V> implements Computable<K, V> { } } } + + public ConcurrentMap<K, Future<V>> getCache() { + return cache; + } } http://git-wip-us.apache.org/repos/asf/tomee/blob/c38893cb/container/openejb-core/src/test/java/org/apache/openejb/util/MemoizerTest.java ---------------------------------------------------------------------- diff --git a/container/openejb-core/src/test/java/org/apache/openejb/util/MemoizerTest.java b/container/openejb-core/src/test/java/org/apache/openejb/util/MemoizerTest.java new file mode 100644 index 0000000..38ccbf4 --- /dev/null +++ b/container/openejb-core/src/test/java/org/apache/openejb/util/MemoizerTest.java @@ -0,0 +1,55 @@ +/* + * 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 org.apache.openejb.util; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class MemoizerTest { + @Test + public void itLeaksByDesignWithWrongEqualsHashCodeAsKey() throws InterruptedException { + final Memoizer<Object[], String> objectArrayCache = new Memoizer<>(new Computable<Object[], String>() { + @Override + public String compute(final Object[] key) throws InterruptedException { + return key[0].toString(); + } + }); + objectArrayCache.compute(new Object[]{"1"}); + assertEquals(1, objectArrayCache.getCache().size()); + + // this is a leak but the way it works, NEVER use an array as key without wrapping it worse case in a list! + objectArrayCache.compute(new Object[]{"1"}); + assertEquals(2, objectArrayCache.getCache().size()); + } + + @Test + public void noLeakWithLoggerKey() throws InterruptedException { + final Memoizer<Logger.LoggerKey, String> objectArrayCache = new Memoizer<>(new Computable<Logger.LoggerKey, String>() { + @Override + public String compute(final Logger.LoggerKey key) throws InterruptedException { + return key.baseName; + } + }); + objectArrayCache.compute(new Logger.LoggerKey(LogCategory.OPENEJB.createChild("test"), "MemoizerTest")); + assertEquals(1, objectArrayCache.getCache().size()); + + // this is a leak but the way it works + objectArrayCache.compute(new Logger.LoggerKey(LogCategory.OPENEJB.createChild("test"), "MemoizerTest") /*new instance*/); + assertEquals(1, objectArrayCache.getCache().size()); + } +}
