From ab8447079fe9efda828cdef2fe810d4c0027ab7e Mon Sep 17 00:00:00 2001 From: neugartf Date: Wed, 25 Sep 2024 02:30:21 +0200 Subject: [PATCH 1/6] Fix ConcurrentModificationException #6732 --- .../api/internal/ConfigUtil.java | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java index 56d2d378d72..4ff6d622a42 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java @@ -5,8 +5,12 @@ package io.opentelemetry.api.internal; +import java.util.AbstractMap; +import java.util.HashSet; import java.util.Locale; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import javax.annotation.Nullable; /** @@ -33,10 +37,21 @@ private ConfigUtil() {} */ public static String getString(String key, String defaultValue) { String normalizedKey = normalizePropertyKey(key); + Set> properties = + new HashSet<>(System.getProperties().entrySet()) + .stream() + .filter( + entry -> entry.getKey() instanceof String && entry.getValue() instanceof String) + .map( + entry -> + new AbstractMap.SimpleEntry<>( + (String) entry.getKey(), (String) entry.getValue())) + .collect(Collectors.>toSet()); + String systemProperty = - System.getProperties().entrySet().stream() - .filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString()))) - .map(entry -> entry.getValue().toString()) + properties.stream() + .filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey()))) + .map(Map.Entry::getValue) .findFirst() .orElse(null); if (systemProperty != null) { From b115c5a6e2f469f437026fad2319ae4588b60cd0 Mon Sep 17 00:00:00 2001 From: neugartf Date: Thu, 26 Sep 2024 21:45:35 +0200 Subject: [PATCH 2/6] Move to stringPropertyNames in order to iterate on a stable list. --- .../opentelemetry/api/internal/ConfigUtil.java | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java index 4ff6d622a42..c4cfde8a1a2 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java @@ -6,7 +6,6 @@ package io.opentelemetry.api.internal; import java.util.AbstractMap; -import java.util.HashSet; import java.util.Locale; import java.util.Map; import java.util.Set; @@ -38,15 +37,12 @@ private ConfigUtil() {} public static String getString(String key, String defaultValue) { String normalizedKey = normalizePropertyKey(key); Set> properties = - new HashSet<>(System.getProperties().entrySet()) - .stream() - .filter( - entry -> entry.getKey() instanceof String && entry.getValue() instanceof String) - .map( - entry -> - new AbstractMap.SimpleEntry<>( - (String) entry.getKey(), (String) entry.getValue())) - .collect(Collectors.>toSet()); + System.getProperties().stringPropertyNames().stream() + .map( + propertyName -> + new AbstractMap.SimpleEntry<>(propertyName, System.getProperty(propertyName))) + .filter(entry -> entry.getKey() != null) + .collect(Collectors.>toSet()); String systemProperty = properties.stream() From af488636cc61f791704f39a47439fb6c4262cb24 Mon Sep 17 00:00:00 2001 From: neugartf Date: Thu, 26 Sep 2024 22:29:20 +0200 Subject: [PATCH 3/6] Improve stream handling of system property --- .../opentelemetry/api/internal/ConfigUtil.java | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java index c4cfde8a1a2..7ddbed24740 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java @@ -5,11 +5,9 @@ package io.opentelemetry.api.internal; -import java.util.AbstractMap; import java.util.Locale; import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; +import java.util.Objects; import javax.annotation.Nullable; /** @@ -36,18 +34,12 @@ private ConfigUtil() {} */ public static String getString(String key, String defaultValue) { String normalizedKey = normalizePropertyKey(key); - Set> properties = - System.getProperties().stringPropertyNames().stream() - .map( - propertyName -> - new AbstractMap.SimpleEntry<>(propertyName, System.getProperty(propertyName))) - .filter(entry -> entry.getKey() != null) - .collect(Collectors.>toSet()); String systemProperty = - properties.stream() - .filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey()))) - .map(Map.Entry::getValue) + System.getProperties().stringPropertyNames().stream() + .filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName))) + .map(System::getProperty) + .filter(Objects::nonNull) .findFirst() .orElse(null); if (systemProperty != null) { From d7364062928d38afccbb3613484cf1789cf37594 Mon Sep 17 00:00:00 2001 From: neugartf Date: Sun, 29 Sep 2024 20:12:33 +0200 Subject: [PATCH 4/6] Cloning properties in order to safely iterate over them --- .../opentelemetry/api/internal/ConfigUtil.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java index 7ddbed24740..304a8ddf740 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java @@ -8,6 +8,7 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.Properties; import javax.annotation.Nullable; /** @@ -35,13 +36,16 @@ private ConfigUtil() {} public static String getString(String key, String defaultValue) { String normalizedKey = normalizePropertyKey(key); + // Cloning in order to avoid ConcurrentModificationException + // see https://github.com/open-telemetry/opentelemetry-java/issues/6732 String systemProperty = - System.getProperties().stringPropertyNames().stream() - .filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName))) - .map(System::getProperty) - .filter(Objects::nonNull) - .findFirst() - .orElse(null); + ((Properties) System.getProperties().clone()) + .stringPropertyNames().stream() + .filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName))) + .map(System::getProperty) + .filter(Objects::nonNull) + .findFirst() + .orElse(null); if (systemProperty != null) { return systemProperty; } From 56ea3da7f5844a1e2ea59dde321140dcebe97b74 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Wed, 30 Oct 2024 17:16:56 -0500 Subject: [PATCH 5/6] Add ConfigUtil#safeSystemProperties() for safe access to system properties --- .../api/internal/ConfigUtil.java | 25 +++++++--- .../api/internal/ConfigUtilTest.java | 49 +++++++++++++++++++ .../spi/internal/DefaultConfigProperties.java | 3 +- 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java index 304a8ddf740..efdcb1549ca 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java @@ -5,9 +5,9 @@ package io.opentelemetry.api.internal; +import java.util.ConcurrentModificationException; import java.util.Locale; import java.util.Map; -import java.util.Objects; import java.util.Properties; import javax.annotation.Nullable; @@ -21,6 +21,17 @@ public final class ConfigUtil { private ConfigUtil() {} + /** + * Returns a copy of system properties which is safe to iterate over. + * + *

In java 8 and android environments, iterating through system properties may trigger {@link + * ConcurrentModificationException}. This method ensures callers can iterate safely without risk + * of exception. See https://github.com/open-telemetry/opentelemetry-java/issues/6732 for details. + */ + public static Properties safeSystemProperties() { + return (Properties) System.getProperties().clone(); + } + /** * Return the system property or environment variable for the {@code key}. * @@ -39,13 +50,11 @@ public static String getString(String key, String defaultValue) { // Cloning in order to avoid ConcurrentModificationException // see https://github.com/open-telemetry/opentelemetry-java/issues/6732 String systemProperty = - ((Properties) System.getProperties().clone()) - .stringPropertyNames().stream() - .filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName))) - .map(System::getProperty) - .filter(Objects::nonNull) - .findFirst() - .orElse(null); + safeSystemProperties().entrySet().stream() + .filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString()))) + .map(entry -> entry.getValue().toString()) + .findFirst() + .orElse(null); if (systemProperty != null) { return systemProperty; } diff --git a/api/all/src/test/java/io/opentelemetry/api/internal/ConfigUtilTest.java b/api/all/src/test/java/io/opentelemetry/api/internal/ConfigUtilTest.java index f7dde6a9735..a546c32862b 100644 --- a/api/all/src/test/java/io/opentelemetry/api/internal/ConfigUtilTest.java +++ b/api/all/src/test/java/io/opentelemetry/api/internal/ConfigUtilTest.java @@ -6,7 +6,15 @@ package io.opentelemetry.api.internal; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.SetSystemProperty; @@ -56,4 +64,45 @@ void defaultIfnull() { assertThat(ConfigUtil.defaultIfNull("val1", "val2")).isEqualTo("val1"); assertThat(ConfigUtil.defaultIfNull(null, "val2")).isEqualTo("val2"); } + + @Test + @SuppressWarnings("ReturnValueIgnored") + void systemPropertiesConcurrentAccess() throws ExecutionException, InterruptedException { + int threads = 4; + ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + int cycles = 1000; + CountDownLatch latch = new CountDownLatch(1); + List> futures = new ArrayList<>(); + for (int i = 0; i < threads; i++) { + futures.add( + executor.submit( + () -> { + try { + latch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + + for (int j = 0; j < cycles; j++) { + String property = "prop " + j; + System.setProperty(property, "a"); + System.getProperties().remove(property); + } + })); + } + + latch.countDown(); + for (int i = 0; i < cycles; i++) { + assertThatCode(() -> ConfigUtil.getString("x", "y")).doesNotThrowAnyException(); + } + + for (Future future : futures) { + future.get(); + } + + } finally { + executor.shutdownNow(); + } + } } diff --git a/sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/DefaultConfigProperties.java b/sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/DefaultConfigProperties.java index af0e4bae2bf..b5496df7c48 100644 --- a/sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/DefaultConfigProperties.java +++ b/sdk-extensions/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/DefaultConfigProperties.java @@ -47,7 +47,8 @@ public final class DefaultConfigProperties implements ConfigProperties { * priority over environment variables. */ public static DefaultConfigProperties create(Map defaultProperties) { - return new DefaultConfigProperties(System.getProperties(), System.getenv(), defaultProperties); + return new DefaultConfigProperties( + ConfigUtil.safeSystemProperties(), System.getenv(), defaultProperties); } /** From 08d09eaefa246fddf088f02ab585fbbc6e3c08a7 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Wed, 30 Oct 2024 17:31:00 -0500 Subject: [PATCH 6/6] remove comment --- .../src/main/java/io/opentelemetry/api/internal/ConfigUtil.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java index efdcb1549ca..3a10a23cb99 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java @@ -47,8 +47,6 @@ public static Properties safeSystemProperties() { public static String getString(String key, String defaultValue) { String normalizedKey = normalizePropertyKey(key); - // Cloning in order to avoid ConcurrentModificationException - // see https://github.com/open-telemetry/opentelemetry-java/issues/6732 String systemProperty = safeSystemProperties().entrySet().stream() .filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString())))