diff --git a/core/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java b/core/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java index 32de2afde369a..a78330c3e8564 100644 --- a/core/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java +++ b/core/src/main/java/org/elasticsearch/common/logging/PrefixLogger.java @@ -26,22 +26,53 @@ import org.apache.logging.log4j.spi.ExtendedLogger; import org.apache.logging.log4j.spi.ExtendedLoggerWrapper; -import java.lang.ref.WeakReference; import java.util.WeakHashMap; +/** + * A logger that prefixes all messages with a fixed prefix specified during construction. The prefix mechanism uses the marker construct, so + * for the prefixes to appear, the logging layout pattern must include the marker in its pattern. + */ class PrefixLogger extends ExtendedLoggerWrapper { - // we can not use the built-in Marker tracking (MarkerManager) because the MarkerManager holds - // a permanent reference to the marker; however, we have transient markers from index-level and - // shard-level components so this would effectively be a memory leak - private static final WeakHashMap> markers = new WeakHashMap<>(); + /* + * We can not use the built-in Marker tracking (MarkerManager) because the MarkerManager holds a permanent reference to the marker; + * however, we have transient markers from index-level and shard-level components so this would effectively be a memory leak. Since we + * can not tie into the lifecycle of these components, we have to use a mechanism that enables garbage collection of such markers when + * they are no longer in use. + */ + private static final WeakHashMap markers = new WeakHashMap<>(); + + /** + * Return the size of the cached markers. This size can vary as markers are cached but collected during GC activity when a given prefix + * is no longer in use. + * + * @return the size of the cached markers + */ + static int markersSize() { + return markers.size(); + } + /** + * The marker for this prefix logger. + */ private final Marker marker; + /** + * Obtain the prefix for this prefix logger. This can be used to create a logger with the same prefix as this one. + * + * @return the prefix + */ public String prefix() { return marker.getName(); } + /** + * Construct a prefix logger with the specified name and prefix. + * + * @param logger the extended logger to wrap + * @param name the name of this prefix logger + * @param prefix the prefix for this prefix logger + */ PrefixLogger(final ExtendedLogger logger, final String name, final String prefix) { super(logger, name, null); @@ -49,11 +80,15 @@ public String prefix() { final Marker actualMarker; // markers is not thread-safe, so we synchronize access synchronized (markers) { - final WeakReference marker = markers.get(actualPrefix); - final Marker maybeMarker = marker == null ? null : marker.get(); + final Marker maybeMarker = markers.get(actualPrefix); if (maybeMarker == null) { actualMarker = new MarkerManager.Log4jMarker(actualPrefix); - markers.put(actualPrefix, new WeakReference<>(actualMarker)); + /* + * We must create a new instance here as otherwise the marker will hold a reference to the key in the weak hash map; as + * those references are held strongly, this would give a strong reference back to the key preventing them from ever being + * collected. This also guarantees that no other strong reference can be held to the prefix anywhere. + */ + markers.put(new String(actualPrefix), actualMarker); } else { actualMarker = maybeMarker; } diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java index cbf160b2ab3de..b03b547e252a8 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/common/logging/EvilLoggerTests.java @@ -47,6 +47,7 @@ import java.util.regex.Pattern; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.startsWith; public class EvilLoggerTests extends ESTestCase { @@ -157,6 +158,20 @@ public void testPrefixLogger() throws IOException, IllegalAccessException, UserE } } + public void testPrefixLoggerMarkersCanBeCollected() throws IOException, UserException { + setupLogging("prefix"); + + final int prefixes = 1 << 19; // to ensure enough markers that the GC should collect some when we force a GC below + for (int i = 0; i < prefixes; i++) { + // this has the side effect of caching a marker with this prefix + Loggers.getLogger("prefix" + i, "prefix" + i); + } + + // this will free the weakly referenced keys in the marker cache + System.gc(); + assertThat(PrefixLogger.markersSize(), lessThan(prefixes)); + } + public void testProperties() throws IOException, UserException { final Settings.Builder builder = Settings.builder().put("cluster.name", randomAlphaOfLength(16)); if (randomBoolean()) {