Skip to content

Commit 78af316

Browse files
committed
Remove caching from PropertiesUtil (+#2849 review)
The `PropertiesUtil` class preemptively caches lookups for all known keys from enumerable property sources. On my Debian system a JVM has around 60 among environment variables and system properties, which causes `PropertiesUtil` to perform around 60 lookups for **each** of the 3 standard property sources. Most of these properties have nothing to do with Log4j. On the other hand all Log4j artifacts use no more than 100 configuration properties and the results of most of those calls are cache in static fields. This PR removes the property value caches used by `PropertiesUtil`. The change should have no noticeable impact on the loading time of Log4j Core, while at the same time simplifies the system.
1 parent f5ddc79 commit 78af316

File tree

9 files changed

+67
-174
lines changed

9 files changed

+67
-174
lines changed

log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilOrderTest.java

Lines changed: 12 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@
3535

3636
@ExtendWith(SystemStubsExtension.class)
3737
@ResourceLock(value = Resources.SYSTEM_PROPERTIES)
38-
public class PropertiesUtilOrderTest {
38+
class PropertiesUtilOrderTest {
3939

40-
public static class NonEnumerablePropertySource implements PropertySource {
40+
private static class NonEnumerablePropertySource implements PropertySource {
4141

4242
private final Properties props;
4343

@@ -67,14 +67,6 @@ public boolean containsProperty(final String key) {
6767
}
6868
}
6969

70-
public static class NullPropertySource implements PropertySource {
71-
72-
@Override
73-
public int getPriority() {
74-
return Integer.MIN_VALUE;
75-
}
76-
}
77-
7870
private final Properties properties = new Properties();
7971

8072
@BeforeEach
@@ -85,7 +77,7 @@ public void setUp() throws Exception {
8577
}
8678

8779
@Test
88-
public void testNormalizedOverrideLegacy() {
80+
void testNormalizedOverrideLegacy() {
8981
final PropertiesUtil util = new PropertiesUtil(properties);
9082
final String legacy = "props.legacy";
9183
final String normalized = "props.normalized";
@@ -104,20 +96,7 @@ public void testNormalizedOverrideLegacy() {
10496
}
10597

10698
@Test
107-
public void testFallsBackToTokenMatching() {
108-
final PropertiesUtil util = new PropertiesUtil(properties);
109-
for (int i = 1; i <= 4; i++) {
110-
final String key = "log4j2.tokenBasedProperty" + i;
111-
assertTrue(util.hasProperty(key));
112-
assertEquals("props.token", util.getStringProperty(key));
113-
}
114-
// No fall back (a normalized property is present)
115-
assertTrue(util.hasProperty("log4j2.normalizedProperty"));
116-
assertEquals("props.normalized", util.getStringProperty("log4j2.normalizedProperty"));
117-
}
118-
119-
@Test
120-
public void testOrderOfNormalizedProperties(final EnvironmentVariables env, final SystemProperties sysProps) {
99+
void testOrderOfNormalizedProperties(final EnvironmentVariables env, final SystemProperties sysProps) {
121100
properties.remove("log4j2.normalizedProperty");
122101
properties.remove("LOG4J_normalized.property");
123102
final PropertiesUtil util = new PropertiesUtil(properties);
@@ -128,29 +107,26 @@ public void testOrderOfNormalizedProperties(final EnvironmentVariables env, fina
128107
assertEquals(null, util.getStringProperty("log4j2.normalizedProperty"));
129108

130109
properties.setProperty("log4j2.normalizedProperty", "props.normalized");
131-
util.reload();
132110
assertTrue(util.hasProperty("Log4jNormalizedProperty"));
133111
assertEquals("props.normalized", util.getStringProperty("Log4jNormalizedProperty"));
134112
assertTrue(util.hasProperty("log4j2.normalizedProperty"));
135113
assertEquals("props.normalized", util.getStringProperty("log4j2.normalizedProperty"));
136114

137115
env.set("LOG4J_NORMALIZED_PROPERTY", "env");
138-
util.reload();
139116
assertTrue(util.hasProperty("Log4jNormalizedProperty"));
140117
assertEquals("env", util.getStringProperty("Log4jNormalizedProperty"));
141118
assertTrue(util.hasProperty("log4j2.normalizedProperty"));
142119
assertEquals("env", util.getStringProperty("log4j2.normalizedProperty"));
143120

144121
sysProps.set("log4j2.normalizedProperty", "sysProps");
145-
util.reload();
146122
assertTrue(util.hasProperty("Log4jNormalizedProperty"));
147123
assertEquals("sysProps", util.getStringProperty("Log4jNormalizedProperty"));
148124
assertTrue(util.hasProperty("log4j2.normalizedProperty"));
149125
assertEquals("sysProps", util.getStringProperty("log4j2.normalizedProperty"));
150126
}
151127

152128
@Test
153-
public void testLegacySystemPropertyHasHigherPriorityThanEnv(
129+
void testLegacySystemPropertyHasHigherPriorityThanEnv(
154130
final EnvironmentVariables env, final SystemProperties sysProps) {
155131
env.set("LOG4J_CONFIGURATION_FILE", "env");
156132
final PropertiesUtil util = new PropertiesUtil(properties);
@@ -159,18 +135,16 @@ public void testLegacySystemPropertyHasHigherPriorityThanEnv(
159135
assertEquals("env", util.getStringProperty("log4j.configurationFile"));
160136

161137
sysProps.set("log4j.configurationFile", "legacy");
162-
util.reload();
163138
assertTrue(util.hasProperty("log4j.configurationFile"));
164139
assertEquals("legacy", util.getStringProperty("log4j.configurationFile"));
165140

166141
sysProps.set("log4j2.configurationFile", "new");
167-
util.reload();
168142
assertTrue(util.hasProperty("log4j.configurationFile"));
169143
assertEquals("new", util.getStringProperty("log4j.configurationFile"));
170144
}
171145

172146
@Test
173-
public void testHighPriorityNonEnumerableSource(final SystemProperties sysProps) {
147+
void testHighPriorityNonEnumerableSource(final SystemProperties sysProps) {
174148
// In both datasources
175149
assertNotNull(properties.getProperty("log4j2.normalizedProperty"));
176150
assertNotNull(properties.getProperty("log4j.onlyLegacy"));
@@ -201,22 +175,19 @@ public void testHighPriorityNonEnumerableSource(final SystemProperties sysProps)
201175
}
202176

203177
/**
204-
* Checks the for missing null checks. The {@link NullPropertySource} returns
178+
* Checks the for missing null checks, using a {@link PropertySource}, which returns
205179
* {@code null} in almost every call.
206-
*
207-
* @param sysProps
208180
*/
209181
@Test
210-
public void testNullChecks(final SystemProperties sysProps) {
182+
void testNullChecks(final SystemProperties sysProps) {
211183
sysProps.set("log4j2.someProperty", "sysProps");
212184
sysProps.set("Log4jLegacyProperty", "sysProps");
213-
final PropertiesUtil util = new PropertiesUtil(new NullPropertySource());
214-
assertTrue(util.hasProperty("log4j2.someProperty"));
215-
assertEquals("sysProps", util.getStringProperty("log4j2.someProperty"));
185+
final PropertiesUtil util = new PropertiesUtil(() -> Integer.MIN_VALUE);
186+
187+
assertTrue(util.hasProperty("Log4jSomeProperty"));
188+
assertEquals("sysProps", util.getStringProperty("log4jSomeProperty"));
216189
assertTrue(util.hasProperty("Log4jLegacyProperty"));
217190
assertEquals("sysProps", util.getStringProperty("Log4jLegacyProperty"));
218-
assertTrue(util.hasProperty("log4j.legacyProperty"));
219-
assertEquals("sysProps", util.getStringProperty("log4j.legacyProperty"));
220191
assertFalse(util.hasProperty("log4j2.nonExistentProperty"));
221192
assertNull(util.getStringProperty("log4j2.nonExistentProperty"));
222193
}

log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -212,15 +212,15 @@ void testBadPropertySource() {
212212
{null, "org.apache.logging.log4j.level"},
213213
{null, "Log4jAnotherProperty"},
214214
{null, "log4j2.catalinaBase"},
215-
{"ok", "log4j2.configurationFile"},
216-
{"ok", "log4j2.defaultStatusLevel"},
217-
{"ok", "log4j2.newLevel"},
218-
{"ok", "log4j2.asyncLoggerTimeout"},
219-
{"ok", "log4j2.asyncLoggerConfigRingBufferSize"},
220-
{"ok", "log4j2.disableThreadContext"},
221-
{"ok", "log4j2.disableThreadContextStack"},
222-
{"ok", "log4j2.disableThreadContextMap"},
223-
{"ok", "log4j2.isThreadContextMapInheritable"}
215+
{"ok", "log4j.configurationFile"},
216+
{"ok", "Log4jDefaultStatusLevel"},
217+
{"ok", "org.apache.logging.log4j.newLevel"},
218+
{"ok", "AsyncLogger.Timeout"},
219+
{"ok", "AsyncLoggerConfig.RingBufferSize"},
220+
{"ok", "disableThreadContext"},
221+
{"ok", "disableThreadContextStack"},
222+
{"ok", "disableThreadContextMap"},
223+
{"ok", "isThreadContextMapInheritable"}
224224
};
225225

226226
/**
@@ -232,7 +232,9 @@ void testBadPropertySource() {
232232
void testResolvesOnlyLog4jProperties() {
233233
final PropertiesUtil util = new PropertiesUtil("Jira3413Test.properties");
234234
for (final String[] pair : data) {
235-
assertEquals(pair[0], util.getStringProperty(pair[1]));
235+
assertThat(util.getStringProperty(pair[1]))
236+
.as("Checking property %s", pair[1])
237+
.isEqualTo(pair[0]);
236238
}
237239
}
238240

log4j-api-test/src/test/resources/Jira3413Test.properties

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,14 @@ another.property=fail
2121
catalina.base=fail
2222

2323
# Some legacy properties with a characteristic prefix
24-
log4j.configurationFile=ok
25-
Log4jDefaultStatusLevel=ok
26-
org.apache.logging.log4j.newLevel=ok
24+
log4j2.configurationFile=ok
25+
log4j2.defaultStatusLevel=ok
26+
log4j2.newLevel=ok
2727

2828
# No characteristic prefix
29-
AsyncLogger.Timeout=ok
30-
AsyncLoggerConfig.RingBufferSize=ok
31-
disableThreadContext=ok
32-
disableThreadContextStack=ok
33-
disableThreadContextMap=ok
34-
isThreadContextMapInheritable=ok
29+
log4j2.asyncLoggerTimeout=ok
30+
log4j2.asyncLoggerConfigRingBufferSize=ok
31+
log4j2.disableThreadContext=ok
32+
log4j2.disableThreadContextStack=ok
33+
log4j2.disableThreadContextMap=ok
34+
log4j2.isThreadContextMapInheritable=ok

0 commit comments

Comments
 (0)