diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java index 2d38a75395f..230d8d15cff 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java @@ -23,10 +23,12 @@ import static java.time.temporal.ChronoUnit.MINUTES; import static java.time.temporal.ChronoUnit.NANOS; import static java.time.temporal.ChronoUnit.SECONDS; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -36,7 +38,6 @@ import java.util.Map; import java.util.Properties; import java.util.stream.Stream; -import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.ResourceAccessMode; @@ -45,19 +46,20 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.junitpioneer.jupiter.Issue; import org.junitpioneer.jupiter.ReadsSystemProperty; -public class PropertiesUtilTest { +class PropertiesUtilTest { private final Properties properties = new Properties(); @BeforeEach - public void setUp() throws Exception { + void setUp() throws Exception { properties.load(ClassLoader.getSystemResourceAsStream("PropertiesUtilTest.properties")); } @Test - public void testExtractSubset() { + void testExtractSubset() { assertHasAllProperties(PropertiesUtil.extractSubset(properties, "a")); assertHasAllProperties(PropertiesUtil.extractSubset(properties, "b.")); assertHasAllProperties(PropertiesUtil.extractSubset(properties, "c.1")); @@ -67,7 +69,7 @@ public void testExtractSubset() { } @Test - public void testPartitionOnCommonPrefix() { + void testPartitionOnCommonPrefix() { final Map parts = PropertiesUtil.partitionOnCommonPrefixes(properties); assertEquals(4, parts.size()); assertHasAllProperties(parts.get("a")); @@ -85,7 +87,7 @@ private static void assertHasAllProperties(final Properties properties) { } @Test - public void testGetCharsetProperty() { + void testGetCharsetProperty() { final Properties p = new Properties(); p.setProperty("e.1", StandardCharsets.US_ASCII.name()); p.setProperty("e.2", "wrong-charset-name"); @@ -130,8 +132,8 @@ static Stream should_properly_parse_duration() { @ParameterizedTest @MethodSource - void should_properly_parse_duration(final Duration expected, final String value) { - Assertions.assertThat(PropertiesUtil.parseDuration(value)).isEqualTo(expected); + void should_properly_parse_duration(final Duration expected, final CharSequence value) { + assertThat(PropertiesUtil.parseDuration(value)).isEqualTo(expected); } static List should_throw_on_invalid_duration() { @@ -142,13 +144,13 @@ static List should_throw_on_invalid_duration() { @ParameterizedTest @MethodSource - void should_throw_on_invalid_duration(final String value) { + void should_throw_on_invalid_duration(final CharSequence value) { assertThrows(IllegalArgumentException.class, () -> PropertiesUtil.parseDuration(value)); } @Test @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ) - public void testGetMappedProperty_sun_stdout_encoding() { + void testGetMappedProperty_sun_stdout_encoding() { final PropertiesUtil pu = new PropertiesUtil(System.getProperties()); final Charset expected = System.console() == null ? Charset.defaultCharset() : StandardCharsets.UTF_8; assertEquals(expected, pu.getCharsetProperty("sun.stdout.encoding")); @@ -156,7 +158,7 @@ public void testGetMappedProperty_sun_stdout_encoding() { @Test @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ) - public void testGetMappedProperty_sun_stderr_encoding() { + void testGetMappedProperty_sun_stderr_encoding() { final PropertiesUtil pu = new PropertiesUtil(System.getProperties()); final Charset expected = System.console() == null ? Charset.defaultCharset() : StandardCharsets.UTF_8; assertEquals(expected, pu.getCharsetProperty("sun.err.encoding")); @@ -164,7 +166,7 @@ public void testGetMappedProperty_sun_stderr_encoding() { @Test @ResourceLock(Resources.SYSTEM_PROPERTIES) - public void testNonStringSystemProperties() { + void testNonStringSystemProperties() { final Object key1 = "1"; final Object key2 = new Object(); System.getProperties().put(key1, new Object()); @@ -180,14 +182,32 @@ public void testNonStringSystemProperties() { @Test @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ) - public void testPublish() { + void testPublish() { final Properties props = new Properties(); - final PropertiesUtil util = new PropertiesUtil(props); + new PropertiesUtil(props); final String value = System.getProperty("Application"); assertNotNull(value, "System property was not published"); assertEquals("Log4j", value); } + @Test + @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ) + @Issue("https://github.com/spring-projects/spring-boot/issues/33450") + void testBadPropertySource() { + final String key = "testKey"; + final Properties props = new Properties(); + props.put(key, "test"); + final PropertiesUtil util = new PropertiesUtil(props); + final ErrorPropertySource source = new ErrorPropertySource(); + util.addPropertySource(source); + try { + assertEquals("test", util.getStringProperty(key)); + assertTrue(source.exceptionThrown); + } finally { + util.removePropertySource(source); + } + } + private static final String[][] data = { {null, "org.apache.logging.log4j.level"}, {null, "Log4jAnotherProperty"}, @@ -209,7 +229,7 @@ public void testPublish() { */ @Test @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ) - public void testResolvesOnlyLog4jProperties() { + void testResolvesOnlyLog4jProperties() { final PropertiesUtil util = new PropertiesUtil("Jira3413Test.properties"); for (final String[] pair : data) { assertEquals(pair[0], util.getStringProperty(pair[1])); @@ -222,7 +242,7 @@ public void testResolvesOnlyLog4jProperties() { */ @Test @ReadsSystemProperty - public void testLog4jProperty() { + void testLog4jProperty() { final Properties props = new Properties(); final String incorrect = "log4j2."; final String correct = "not.starting.with.log4j"; @@ -231,4 +251,40 @@ public void testLog4jProperty() { final PropertiesUtil util = new PropertiesUtil(props); assertEquals(correct, util.getStringProperty(correct)); } + + @Test + void should_support_multiple_sources_with_same_priority() { + final int priority = 2003; + final String key1 = "propertySource1"; + final Properties props1 = new Properties(); + props1.put(key1, "props1"); + final String key2 = "propertySource2"; + final Properties props2 = new Properties(); + props2.put(key2, "props2"); + final PropertiesUtil util = new PropertiesUtil(new PropertiesPropertySource(props1, priority)); + util.addPropertySource(new PropertiesPropertySource(props2, priority)); + assertThat(util.getStringProperty(key1)).isEqualTo("props1"); + assertThat(util.getStringProperty(key2)).isEqualTo("props2"); + } + + private static class ErrorPropertySource implements PropertySource { + public boolean exceptionThrown = false; + + @Override + public int getPriority() { + return Integer.MIN_VALUE; + } + + @Override + public String getProperty(final String key) { + exceptionThrown = true; + throw new IllegalStateException("Test"); + } + + @Override + public boolean containsProperty(final String key) { + exceptionThrown = true; + throw new IllegalStateException("Test"); + } + } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java index d3fd0952d92..fdcee68eb82 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java @@ -36,13 +36,16 @@ import java.util.ServiceLoader; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentSkipListSet; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.PropertySource.Comparator; /** * Consider this class private. @@ -139,13 +142,21 @@ public static PropertiesUtil getProperties() { } /** - * Allows a PropertySource to be added after PropertiesUtil has been created. - * @param propertySource the PropertySource to add. + * Allows a {@link PropertySource} to be added after {@code PropertiesUtil} has been created. + * @param propertySource the {@code PropertySource} to add. + * @since 2.19.0 */ public void addPropertySource(final PropertySource propertySource) { - if (environment != null) { - environment.addPropertySource(propertySource); - } + environment.addPropertySource(Objects.requireNonNull(propertySource)); + } + + /** + * Removes a {@link PropertySource}. + * @param propertySource the {@code PropertySource} to remove. + * @since 2.24.0 + */ + public void removePropertySource(final PropertySource propertySource) { + environment.removePropertySource(Objects.requireNonNull(propertySource)); } /** @@ -488,7 +499,11 @@ public void reload() { */ private static final class Environment { - private final Set sources = new ConcurrentSkipListSet<>(new PropertySource.Comparator()); + private final List sources = new ArrayList<>(); + private final ReadWriteLock sourceLock = new ReentrantReadWriteLock(); + private final Lock sourceReadLock = sourceLock.readLock(); + private final Lock sourceWriteLock = sourceLock.writeLock(); + /** * Maps a key to its value or the value of its normalization in the lowest priority source that contains it. */ @@ -517,45 +532,64 @@ private Environment(final PropertySource propertySource) { ServiceLoader.load(PropertySource.class, PropertiesUtil.class.getClassLoader()), LOGGER) .forEach(sources::add); + sources.sort(Comparator.INSTANCE); reload(); } - /** - * Allow a PropertySource to be added. - * @param propertySource The PropertySource to add. - */ - public void addPropertySource(final PropertySource propertySource) { - sources.add(propertySource); + private void addPropertySource(final PropertySource propertySource) { + sourceWriteLock.lock(); + try { + if (!sources.contains(propertySource)) { + sources.add(propertySource); + sources.sort(Comparator.INSTANCE); + } + } finally { + sourceWriteLock.unlock(); + } + } + + private void removePropertySource(final PropertySource propertySource) { + sourceWriteLock.lock(); + try { + sources.remove(propertySource); + } finally { + sourceWriteLock.unlock(); + } } - private synchronized void reload() { + private void reload() { literal.clear(); tokenized.clear(); - // 1. Collects all property keys from enumerable sources. - final Set keys = new HashSet<>(); - sources.stream().map(PropertySource::getPropertyNames).forEach(keys::addAll); - // 2. Fills the property caches. Sources with higher priority values don't override the previous ones. - keys.stream().filter(Strings::isNotBlank).forEach(key -> { - final List tokens = PropertySource.Util.tokenize(key); - final boolean hasTokens = !tokens.isEmpty(); - sources.forEach(source -> { - if (source.containsProperty(key)) { - final String value = source.getProperty(key); - if (hasTokens) { - tokenized.putIfAbsent(tokens, value); + sourceWriteLock.lock(); + try { + // 1. Collects all property keys from enumerable sources. + final Set keys = new HashSet<>(); + sources.stream().map(PropertySource::getPropertyNames).forEach(keys::addAll); + // 2. Fills the property caches. Sources with higher priority values don't override the previous ones. + keys.stream().filter(Strings::isNotBlank).forEach(key -> { + final List tokens = PropertySource.Util.tokenize(key); + final boolean hasTokens = !tokens.isEmpty(); + sources.forEach(source -> { + if (sourceContainsProperty(source, key)) { + final String value = sourceGetProperty(source, key); + if (hasTokens) { + tokenized.putIfAbsent(tokens, value); + } } - } - if (hasTokens) { - final String normalKey = Objects.toString(source.getNormalForm(tokens), null); - if (normalKey != null && source.containsProperty(normalKey)) { - literal.putIfAbsent(key, source.getProperty(normalKey)); - } else if (source.containsProperty(key)) { - literal.putIfAbsent(key, source.getProperty(key)); + if (hasTokens) { + final String normalKey = Objects.toString(source.getNormalForm(tokens), null); + if (normalKey != null && sourceContainsProperty(source, normalKey)) { + literal.putIfAbsent(key, sourceGetProperty(source, normalKey)); + } else if (sourceContainsProperty(source, key)) { + literal.putIfAbsent(key, sourceGetProperty(source, key)); + } } - } + }); }); - }); + } finally { + sourceWriteLock.unlock(); + } } private String get(final String key) { @@ -564,29 +598,57 @@ private String get(final String key) { } final List tokens = PropertySource.Util.tokenize(key); final boolean hasTokens = !tokens.isEmpty(); - for (final PropertySource source : sources) { - if (hasTokens) { - final String normalKey = Objects.toString(source.getNormalForm(tokens), null); - if (normalKey != null && source.containsProperty(normalKey)) { - return source.getProperty(normalKey); + sourceReadLock.lock(); + try { + for (final PropertySource source : sources) { + if (hasTokens) { + final String normalKey = Objects.toString(source.getNormalForm(tokens), null); + if (normalKey != null && sourceContainsProperty(source, normalKey)) { + return sourceGetProperty(source, normalKey); + } + } + if (sourceContainsProperty(source, key)) { + return sourceGetProperty(source, key); } } - if (source.containsProperty(key)) { - return source.getProperty(key); - } + } finally { + sourceReadLock.unlock(); } return tokenized.get(tokens); } + private boolean sourceContainsProperty(final PropertySource source, final String key) { + try { + return source.containsProperty(key); + } catch (final Exception e) { + LOGGER.warn("Failed to retrieve Log4j property {} from property source {}.", key, source, e); + return false; + } + } + + private String sourceGetProperty(final PropertySource source, final String key) { + try { + return source.getProperty(key); + } catch (final Exception e) { + LOGGER.warn("Failed to retrieve Log4j property {} from property source {}.", key, source, e); + return null; + } + } + private boolean containsKey(final String key) { final List tokens = PropertySource.Util.tokenize(key); - return literal.containsKey(key) - || tokenized.containsKey(tokens) - || sources.stream().anyMatch(s -> { - final CharSequence normalizedKey = s.getNormalForm(tokens); - return s.containsProperty(key) - || (normalizedKey != null && s.containsProperty(normalizedKey.toString())); - }); + sourceReadLock.lock(); + try { + return literal.containsKey(key) + || tokenized.containsKey(tokens) + || sources.stream().anyMatch(s -> { + final CharSequence normalizedKey = s.getNormalForm(tokens); + return sourceContainsProperty(s, key) + || (normalizedKey != null && sourceContainsProperty(s, normalizedKey.toString())); + }); + } finally { + sourceReadLock.unlock(); + } } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertySource.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertySource.java index 6e76aabd3d5..399c4ea010c 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertySource.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertySource.java @@ -50,7 +50,7 @@ public interface PropertySource { * * @param action action to perform on each key/value pair */ - default void forEach(BiConsumer action) {} + default void forEach(final BiConsumer action) {} /** * Returns the list of all property names. @@ -68,7 +68,7 @@ default Collection getPropertyNames() { * @param tokens list of property name tokens * @return a normalized property name using the given tokens */ - default CharSequence getNormalForm(Iterable tokens) { + default CharSequence getNormalForm(final Iterable tokens) { return null; } @@ -78,7 +78,7 @@ default CharSequence getNormalForm(Iterable tokens) { * @return The value or null; * @since 2.13.0 */ - default String getProperty(String key) { + default String getProperty(final String key) { return null; } @@ -88,7 +88,7 @@ default String getProperty(String key) { * @return The value or null; * @since 2.13.0 */ - default boolean containsProperty(String key) { + default boolean containsProperty(final String key) { return false; } @@ -100,6 +100,8 @@ default boolean containsProperty(String key) { class Comparator implements java.util.Comparator, Serializable { private static final long serialVersionUID = 1L; + static final Comparator INSTANCE = new Comparator(); + @Override public int compare(final PropertySource o1, final PropertySource o2) { return Integer.compare( diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/Server.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/Server.java index d5adfb10bb3..c96721dfc0b 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/Server.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/Server.java @@ -237,12 +237,14 @@ private static ContextSelector getContextSelector() { * @param loggerContextName name of the logger context to unregister */ public static void unregisterLoggerContext(final String loggerContextName) { - if (isJmxDisabled()) { - LOGGER.debug("JMX disabled for Log4j2. Not unregistering MBeans."); - return; + if (loggerContextName != null) { + if (isJmxDisabled()) { + LOGGER.debug("JMX disabled for Log4j2. Not unregistering MBeans."); + return; + } + final MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); + unregisterLoggerContext(loggerContextName, mbs); } - final MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); - unregisterLoggerContext(loggerContextName, mbs); } /** diff --git a/src/changelog/.2.x.x/1799_ignore_propertysource_errors.xml b/src/changelog/.2.x.x/1799_ignore_propertysource_errors.xml new file mode 100644 index 00000000000..e4281a0a8b2 --- /dev/null +++ b/src/changelog/.2.x.x/1799_ignore_propertysource_errors.xml @@ -0,0 +1,8 @@ + + + + Ignore exceptions thrown by PropertySources. + diff --git a/src/changelog/.2.x.x/LOG4J2-3618_propertysource_comparator.xml b/src/changelog/.2.x.x/LOG4J2-3618_propertysource_comparator.xml new file mode 100644 index 00000000000..04911e98587 --- /dev/null +++ b/src/changelog/.2.x.x/LOG4J2-3618_propertysource_comparator.xml @@ -0,0 +1,8 @@ + + + + Fixes property source ordering to account for same priority of different sources. +