This is an automated email from the ASF dual-hosted git repository.
ddekany pushed a commit to branch FREEMARKER-35
in repository https://gitbox.apache.org/repos/asf/freemarker.git
The following commit(s) were added to refs/heads/FREEMARKER-35 by this push:
new fdda2ffc FREEMARKER-35: Added memory leak (forever growing cache)
protection to Environment-level by-pattern Temporal format cache. Added tests
for format cache flushing, both for this cache, and for the global caches in
format factories.
fdda2ffc is described below
commit fdda2ffcb84f2cc0168677c9346bdaa7c3d35891
Author: ddekany <ddek...@apache.org>
AuthorDate: Sun Aug 28 18:12:58 2022 +0200
FREEMARKER-35: Added memory leak (forever growing cache) protection to
Environment-level by-pattern Temporal format cache. Added tests for format
cache flushing, both for this cache, and for the global caches in format
factories.
---
src/main/java/freemarker/core/Environment.java | 54 +++++++++-
.../java/freemarker/core/FastLRUKeyValueStore.java | 21 +++-
.../core/JavaTemplateDateFormatFactory.java | 36 +++++--
.../core/JavaTemplateNumberFormatFactory.java | 31 +++++-
.../core/JavaTemplateTemporalFormatFactory.java | 17 ++-
...ormatCacheMemoryLeakPreventionFlushingTest.java | 114 +++++++++++++++++++++
6 files changed, 253 insertions(+), 20 deletions(-)
diff --git a/src/main/java/freemarker/core/Environment.java
b/src/main/java/freemarker/core/Environment.java
index a5ef0b1a..93a41166 100644
--- a/src/main/java/freemarker/core/Environment.java
+++ b/src/main/java/freemarker/core/Environment.java
@@ -143,6 +143,12 @@ public final class Environment extends Configurable {
C_NUMBER_FORMAT_ICI_2_3_21.setDecimalFormatSymbols(symbols);
}
+ /**
+ * Maximum number of patterns per class in {@link
#cachedTemporalFormatsByFormatString}, after which cache size
+ * must be reduced. Not private to help unit testing
+ */
+ static final int
CACHED_TEMPORAL_FORMATS_BY_FORMAT_STRING_MAX_SIZE_PER_CLASS = 128;
+
private final Configuration configuration;
private final boolean incompatibleImprovementsGE2328;
private final TemplateHashModel rootDataModel;
@@ -2901,15 +2907,57 @@ public final class Environment extends Configurable {
throw new BugException("Unhandled temporal class: " +
temporalClass.getName());
}
- return classSpecificMap.computeIfAbsent(
- formatString,
- k -> new
TemplateTemporalFormat[CACHED_TEMPORAL_FORMATS_BY_FORMAT_STRING_VALUE_ARRAY_LENGTH]);
+ TemplateTemporalFormat[] cachedFormats =
classSpecificMap.get(formatString);
+ if (cachedFormats != null) {
+ return cachedFormats;
+ }
+
+ // To defend against memory leak if some misbehaving template
generates lots of unique formats:
+ if (classSpecificMap.size() >=
CACHED_TEMPORAL_FORMATS_BY_FORMAT_STRING_MAX_SIZE_PER_CLASS) {
+ classSpecificMap.clear();
+ }
+
+ cachedFormats = new
TemplateTemporalFormat[CACHED_TEMPORAL_FORMATS_BY_FORMAT_STRING_VALUE_ARRAY_LENGTH];
+ classSpecificMap.put(formatString, cachedFormats);
+ return cachedFormats;
}
+ /** Exposed for unit testing */
void clearCachedTemplateTemporalFormatsByFormatString() {
cachedTemporalFormatsByFormatString = null;
}
+ /** This is used for unit testing */
+ int getTemplateTemporalFormatsByFormatStringMapSize(Class<? extends
Temporal> temporalClass) {
+ if (cachedTemporalFormatsByFormatString == null) {
+ return 0;
+ }
+
+ Map<String, TemplateTemporalFormat[]> classSpecificMap;
+ if (temporalClass == Instant.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.instantFormats;
+ } else if (temporalClass == LocalDate.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.localDateFormats;
+ } else if (temporalClass == LocalDateTime.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.localDateTimeFormats;
+ } else if (temporalClass == LocalTime.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.localTimeFormats;
+ } else if (temporalClass == OffsetDateTime.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.offsetDateTimeFormats;
+ } else if (temporalClass == OffsetTime.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.offsetTimeFormats;
+ } else if (temporalClass == ZonedDateTime.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.zonedDateTimeFormats;
+ } else if (temporalClass == YearMonth.class) {
+ classSpecificMap =
cachedTemporalFormatsByFormatString.yearMonthFormats;
+ } else if (temporalClass == Year.class) {
+ classSpecificMap = cachedTemporalFormatsByFormatString.yearFormats;
+ } else {
+ throw new BugException("Unhandled temporal class: " +
temporalClass.getName());
+ }
+ return classSpecificMap != null ? classSpecificMap.size() : 0;
+ }
+
/**
* Returns the {@link TemplateTemporalFormat} for the given parameters
without using any
* {@link Environment}-level cacheing. The {@link
TemplateTemporalFormatFactory} involved might still uses its
diff --git a/src/main/java/freemarker/core/FastLRUKeyValueStore.java
b/src/main/java/freemarker/core/FastLRUKeyValueStore.java
index 9cbf465a..20508258 100644
--- a/src/main/java/freemarker/core/FastLRUKeyValueStore.java
+++ b/src/main/java/freemarker/core/FastLRUKeyValueStore.java
@@ -67,6 +67,8 @@ class FastLRUKeyValueStore<K, V> {
}
/**
+ * Gets the entry with the given key, and ensures that it becomes a
"recent" entry.
+ *
* @return {@code null} if there's no entry with the given key.
*/
V get(K cacheKey) {
@@ -83,8 +85,23 @@ class FastLRUKeyValueStore<K, V> {
return putIfAbsentThenReturnStored(cacheKey, value);
}
+ /**
+ * Drops all entries.
+ */
void clear() {
- olderEntries.clear();
- recentEntries.clear();
+ synchronized (this) {
+ olderEntries.clear();
+ recentEntries.clear();
+ }
+ }
+
+ /**
+ * Total number of entries stored at the moment. Can be inaccurate if the
store concurrent modified, but the main
+ * application is unit testing, where we can avoid that.
+ */
+ int size() {
+ synchronized (this) {
+ return recentEntries.size() + olderEntries.size();
+ }
}
}
diff --git a/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java
b/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java
index f4773ac3..09a8ab8d 100644
--- a/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java
+++ b/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java
@@ -25,24 +25,27 @@ import java.util.Locale;
import java.util.StringTokenizer;
import java.util.TimeZone;
-import freemarker.log.Logger;
import freemarker.template.TemplateDateModel;
class JavaTemplateDateFormatFactory extends TemplateDateFormatFactory {
-
- static final JavaTemplateDateFormatFactory INSTANCE = new
JavaTemplateDateFormatFactory();
-
- private static final Logger LOG = Logger.getLogger("freemarker.runtime");
- private final FastLRUKeyValueStore<CacheKey, DateFormat> dateFormatCache =
new FastLRUKeyValueStore<>(512);
+ static final JavaTemplateDateFormatFactory INSTANCE = new
JavaTemplateDateFormatFactory();
+
+ /**
+ * Exposed to unit testing.
+ */
+ static final int GUARANTEED_RECENT_ENTRIES = 512;
+
+ private final FastLRUKeyValueStore<CacheKey, DateFormat> dateFormatCache =
+ new FastLRUKeyValueStore<>(GUARANTEED_RECENT_ENTRIES);
private JavaTemplateDateFormatFactory() {
// Can't be instantiated
}
-
+
/**
* @param zonelessInput
- * Has no effect in this implementation.
+ * Has no effect in this implementation.
*/
@Override
public TemplateDateFormat get(String params, int dateType, Locale locale,
TimeZone timeZone, boolean zonelessInput,
@@ -101,6 +104,21 @@ class JavaTemplateDateFormatFactory extends
TemplateDateFormatFactory {
}
}
+
+ /**
+ * Used for unit testing.
+ */
+ void clear() {
+ dateFormatCache.clear();
+ }
+
+ /**
+ * Used for unit testing.
+ */
+ int getSize() {
+ return dateFormatCache.size();
+ }
+
private static final class CacheKey {
private final int dateType;
private final String pattern;
@@ -145,5 +163,5 @@ class JavaTemplateDateFormatFactory extends
TemplateDateFormatFactory {
}
return -1;
}
-
+
}
diff --git a/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java
b/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java
index a3de36a3..5ad814d3 100644
--- a/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java
+++ b/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java
@@ -26,17 +26,23 @@ import java.util.Locale;
* Deals with {@link TemplateNumberFormat}-s that just wrap a Java {@link
NumberFormat}.
*/
class JavaTemplateNumberFormatFactory extends TemplateNumberFormatFactory {
-
+
static final JavaTemplateNumberFormatFactory INSTANCE = new
JavaTemplateNumberFormatFactory();
static final String COMPUTER = "computer";
- private final FastLRUKeyValueStore<CacheKey, NumberFormat>
numberFormatCache = new FastLRUKeyValueStore<>(512);
+ /**
+ * Exposed to unit testing.
+ */
+ static final int GUARANTEED_RECENT_ENTRIES = 512;
+
+ private final FastLRUKeyValueStore<CacheKey, NumberFormat>
numberFormatCache = new FastLRUKeyValueStore<>(
+ GUARANTEED_RECENT_ENTRIES);
private JavaTemplateNumberFormatFactory() {
// Not meant to be instantiated
}
-
+
@Override
public TemplateNumberFormat get(String params, Locale locale, Environment
env)
throws InvalidFormatParametersException {
@@ -49,7 +55,7 @@ class JavaTemplateNumberFormatFactory extends
TemplateNumberFormatFactory {
numberFormat = getNumberFormatNoCache(params, locale, env);
numberFormat =
numberFormatCache.putIfAbsentThenReturnStored(cacheKey, numberFormat);
}
-
+
return new JavaTemplateNumberFormat((NumberFormat)
numberFormat.clone(), params);
}
@@ -74,6 +80,21 @@ class JavaTemplateNumberFormatFactory extends
TemplateNumberFormatFactory {
}
}
+
+ /**
+ * Used for unit testing.
+ */
+ void clear() {
+ numberFormatCache.clear();
+ }
+
+ /**
+ * Used for unit testing.
+ */
+ int getSize() {
+ return numberFormatCache.size();
+ }
+
private static final class CacheKey {
private final String pattern;
private final Locale locale;
@@ -97,5 +118,5 @@ class JavaTemplateNumberFormatFactory extends
TemplateNumberFormatFactory {
return pattern.hashCode() ^ locale.hashCode();
}
}
-
+
}
diff --git
a/src/main/java/freemarker/core/JavaTemplateTemporalFormatFactory.java
b/src/main/java/freemarker/core/JavaTemplateTemporalFormatFactory.java
index 2ad345c3..b88c0dc8 100644
--- a/src/main/java/freemarker/core/JavaTemplateTemporalFormatFactory.java
+++ b/src/main/java/freemarker/core/JavaTemplateTemporalFormatFactory.java
@@ -27,8 +27,13 @@ import java.util.TimeZone;
class JavaTemplateTemporalFormatFactory extends TemplateTemporalFormatFactory {
public static final JavaTemplateTemporalFormatFactory INSTANCE = new
JavaTemplateTemporalFormatFactory();
+ /**
+ * Exposed to unit testing.
+ */
+ static final int GUARANTEED_RECENT_ENTRIES = 512;
+
private final FastLRUKeyValueStore<CacheKey, JavaTemplateTemporalFormat>
formatCache =
- new FastLRUKeyValueStore<>(512);
+ new FastLRUKeyValueStore<>(GUARANTEED_RECENT_ENTRIES);
private JavaTemplateTemporalFormatFactory() {
// Not instantiated from outside
@@ -49,10 +54,20 @@ class JavaTemplateTemporalFormatFactory extends
TemplateTemporalFormatFactory {
return format;
}
+ /**
+ * Used for unit testing.
+ */
void clear() {
formatCache.clear();
}
+ /**
+ * Used for unit testing.
+ */
+ int getSize() {
+ return formatCache.size();
+ }
+
private static class CacheKey {
private final String params;
private final Class<? extends Temporal> temporalClass;
diff --git
a/src/test/java/freemarker/core/FormatCacheMemoryLeakPreventionFlushingTest.java
b/src/test/java/freemarker/core/FormatCacheMemoryLeakPreventionFlushingTest.java
new file mode 100644
index 00000000..b0c1e6d9
--- /dev/null
+++
b/src/test/java/freemarker/core/FormatCacheMemoryLeakPreventionFlushingTest.java
@@ -0,0 +1,114 @@
+/*
+ * 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 freemarker.core;
+
+import static org.junit.Assert.*;
+
+import java.time.Instant;
+
+import org.junit.Test;
+
+import freemarker.template.TemplateDateModel;
+
+/**
+ * This is to cover the very rare situation where a format cache grows too
big, and we fo some sort of flushing to
+ * prevent a memory leak. The main goals is to catch any exception causing bug
in that rarely ran code branch. But we
+ * also assert when exactly the flushing happens; that can change in the
future, in which case adjust the tests.
+ */
+public class FormatCacheMemoryLeakPreventionFlushingTest
+ extends TemplateTemporalFormatAbstractCachingInEnvironmentTest {
+
+ @Test
+ public void testGlobalJavaTemporalFormatCacheLeakProtection() throws
Exception {
+ JavaTemplateTemporalFormatFactory formatFactory =
JavaTemplateTemporalFormatFactory.INSTANCE;
+ formatFactory.clear();
+ int expectedMapSize = 0;
+ for (int i = 0; i <
JavaTemplateTemporalFormatFactory.GUARANTEED_RECENT_ENTRIES * 3 * 2 + 1; i++) {
+ assertEquals(expectedMapSize, formatFactory.getSize());
+ formatFactory.get(
+ "yyyyMMddHHmm '" + i + "'",
+ Instant.class, env.getLocale(), env.getTimeZone(), env);
+ expectedMapSize++;
+ // Following builds on implementation details (not on API
contract), so adjust this if that changes.
+ if (expectedMapSize >
JavaTemplateTemporalFormatFactory.GUARANTEED_RECENT_ENTRIES * 2) {
+ // The flushed entries are kept, and just become "old"
entries, therefore after flushing we have
+ // GUARANTEED_RECENT_ENTRIES entries. Then the new entry that
caused the flushing is added to the
+ // "recent" entries, hence the +1.
+ expectedMapSize =
JavaTemplateTemporalFormatFactory.GUARANTEED_RECENT_ENTRIES + 1;
+ }
+ }
+ }
+
+ @Test
+ public void testGlobalJavaDateFormatCacheLeakProtection() throws Exception
{
+ JavaTemplateDateFormatFactory formatFactory =
JavaTemplateDateFormatFactory.INSTANCE;
+ formatFactory.clear();
+ int expectedMapSize = 0;
+ for (int i = 0; i <
JavaTemplateDateFormatFactory.GUARANTEED_RECENT_ENTRIES * 3 * 2 + 1; i++) {
+ assertEquals(expectedMapSize, formatFactory.getSize());
+ formatFactory.get(
+ "yyyyMMddHHmm '" + i + "'",
+ TemplateDateModel.DATETIME, env.getLocale(),
env.getTimeZone(), false, env);
+ expectedMapSize++;
+ // Following builds on implementation details (not on API
contract), so adjust this if that changes.
+ if (expectedMapSize >
JavaTemplateDateFormatFactory.GUARANTEED_RECENT_ENTRIES * 2) {
+ // The flushed entries are kept, and just become "old"
entries, therefore after flushing we have
+ // GUARANTEED_RECENT_ENTRIES entries. Then the new entry that
caused the flushing is added to the
+ // "recent" entries, hence the +1.
+ expectedMapSize =
JavaTemplateDateFormatFactory.GUARANTEED_RECENT_ENTRIES + 1;
+ }
+ }
+ }
+
+ @Test
+ public void testGlobalNumberFormatCacheLeakProtection() throws Exception {
+ JavaTemplateNumberFormatFactory formatFactory =
JavaTemplateNumberFormatFactory.INSTANCE;
+ formatFactory.clear();
+ int expectedMapSize = 0;
+ for (int i = 0; i <
JavaTemplateNumberFormatFactory.GUARANTEED_RECENT_ENTRIES * 3 * 2 + 1; i++) {
+ assertEquals(expectedMapSize, formatFactory.getSize());
+ formatFactory.get(
+ "#.# '" + i + "'",
+ env.getLocale(), env);
+ expectedMapSize++;
+ // Following builds on implementation details (not on API
contract), so adjust this if that changes.
+ if (expectedMapSize >
JavaTemplateNumberFormatFactory.GUARANTEED_RECENT_ENTRIES * 2) {
+ // The flushed entries are kept, and just become "old"
entries, therefore after flushing we have
+ // GUARANTEED_RECENT_ENTRIES entries. Then the new entry that
caused the flushing is added to the
+ // "recent" entries, hence the +1.
+ expectedMapSize =
JavaTemplateNumberFormatFactory.GUARANTEED_RECENT_ENTRIES + 1;
+ }
+ }
+ }
+
+ @Test
+ public void testEnvironmentLevelByFormatCacheMemoryLeakProtection() throws
Exception {
+ env.clearCachedTemplateTemporalFormatsByFormatString();
+ int expectedMapSize = 0;
+ for (int i = 0; i <
Environment.CACHED_TEMPORAL_FORMATS_BY_FORMAT_STRING_MAX_SIZE_PER_CLASS * 3 +
1; i++) {
+ assertEquals(expectedMapSize,
env.getTemplateTemporalFormatsByFormatStringMapSize(Instant.class));
+ env.getTemplateTemporalFormat("yyyyMMddHHmm '" + i + "'",
Instant.class);
+ expectedMapSize++;
+ if (expectedMapSize >
Environment.CACHED_TEMPORAL_FORMATS_BY_FORMAT_STRING_MAX_SIZE_PER_CLASS) {
+ expectedMapSize = 1; // Because we should store the new entry
that caused the flushing
+ }
+ }
+ }
+}
