Fix ConcurrentModificationException #6732

[neugartf]

Could be nicer with jdk > 8, but that's how it is :). I added a filter to make sure only string key/values from Properties are read into our logic.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: neugartf / name: Fabian Neugart (ab84470, b115c5a, af48863, 329c05d, d736406)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.06%. Comparing base (697b4e0) to head (d736406).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6746      +/-   ##
============================================
- Coverage     90.08%   90.06%   -0.02%     
+ Complexity     6464     6462       -2     
============================================
  Files           718      718              
  Lines         19528    19530       +2     
  Branches       1923     1923              
============================================
- Hits          17591    17590       -1     
- Misses         1348     1350       +2     
- Partials        589      590       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[laurit]

stringPropertyNames calls entrySet without extra synchronization so ConcurrentModificationException should still be possible

[neugartf]

stringPropertyNames promises an unmodifiable Set<String> based on enumerateStringProperties(), which at least in JDK8 seems to be synchronized.

Any other idea then? The last that comes to my mind is using clone() and do the operation on the copy. I'm all ears!

[laurit]

In jdk11 that method is not synchronized.

[neugartf]

Yeah, super weird.
Was also checking the Android implementation, same thing.
https://cs.android.com/android/platform/superproject/main/+/main:libcore/ojluni/src/main/java/java/util/Properties.java

[trask]

does enumerating over System.getProperties().keys() help?

[neugartf]

Added commit with clone(). This did prevent the exception (at least in jdk 8).

[trask]

clone() is going to be a lot more expensive

fwiw keys() appears to be marked as synchronized: https://github.com/openjdk/jdk/blob/9a9add8825a040565051a09010b29b099c2e7d49/jdk/src/share/classes/java/util/Hashtable.java#L256

[laurit]

fwiw keys() appears to be marked as synchronized

I don't think that is enough. https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Hashtable.html says

The iterators returned by the iterator method of the collections returned by all of this class's "collection view methods" are fail-fast: if the Hashtable is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove method, the iterator will throw a ConcurrentModificationException. Thus, in the face of concurrent modification, the iterator fails quickly and cleanly, rather than risking arbitrary, non-deterministic behavior at an undetermined time in the future. The Enumerations returned by Hashtable's keys and elements methods are not fail-fast; if the Hashtable is structurally modified at any time after the enumeration is created then the results of enumerating are undefined.

Similarly to how you can use iterator from synchronized wrapped created with Collections.synchronized... safely by synchronizing on the collection you could do the same here.

Properties properties = System.getProperties();
synchronized (properties) {
  String systemProperty =
      properties.entrySet().stream()
          .filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString())))
          .map(entry -> entry.getValue().toString())
          .findFirst()
          .orElse(null);
  if (systemProperty != null) {
    return systemProperty;
  }
}

Alternatively you could reduce the chance of race with making a copy of system properties once.

  @SuppressWarnings({"NullAway", "NonFinalStaticField"})
  private static Map<String, String> systemProperties;
  static {
    initialize();
  }

  // Visible for testing
  static void initialize() {
    systemProperties = System.getProperties().entrySet().stream().collect(
        Collectors.toMap(entry -> normalizePropertyKey(entry.getKey().toString()),
            entry -> entry.getValue().toString()));
  }

  public static String getString(String key, String defaultValue) {
    String normalizedKey = normalizePropertyKey(key);
    String systemProperty = systemProperties.get(normalizedKey);
    if (systemProperty != null) {
      return systemProperty;
    }
    ...
  }

with this you'll lose the ability to pick up modifications to system properties. I doubt that this is actually something we need outside of tests, where you can call initialize again. Before implementing this approach I'd ask @jack-berg whether this is fine with him.

[trask]

Alternatively you could reduce the chance of race with making a copy of system properties once.

with this you'll lose the ability to pick up modifications to system properties

if we can scope this one time reading of system properties once per SDK creation that seems ideal (so you can set system properties and then instantiate the SDK)

[jack-berg]

if we can scope this one time reading of system properties once per SDK creation that seems ideal (so you can set system properties and then instantiate the SDK)

so something like:

  private static final Properties systemProperties = (Properties) System.getProperties().clone();

??

dismiss

[shalk]

how about System.getProperties().stringPropertyNames() is more light than clone

[shalk]

Suggested change

	import java.util.Locale;
	import java.util.Map;
	import java.util.Objects;
	import java.util.Properties;
	import java.util.Collections;
	import java.util.Locale;
	import java.util.Map;

[shalk]

Suggested change

	((Properties) System.getProperties().clone())
	.stringPropertyNames().stream()
	.filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
	.map(System::getProperty)
	.filter(Objects::nonNull)
	.findFirst()
	.orElse(null);
	String systemProperty =
	Collections.unmodifiableSet(System.getProperties().stringPropertyNames()).stream()
	.filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
	.map(System::getProperty)
	.findFirst()
	.orElse(null);

[jack-berg]

unmodifiable set is superfluous since there's no attempt to modify the contents.

[shalk]

I think we can add a hook method in stream pipeline to support unit test

hook method is like this:

  private <T> Function< T,  T> hook() {
    return new Function<T, T>() {
      @Override
      public T apply(T t) {
        System.setProperty("ConfigUtilTestKey" , "ConfigUtilTestKey");
        System.clearProperty("ConfigUtilTestKey" );
        return t;
      }
    };
  }

origin code can be like this:

    String systemProperty = Collections.unmodifiableSet(System.getProperties().stringPropertyNames()).stream()
        .map(hook())
        .filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
        .map(System::getProperty)
        .findFirst()
        .orElse(null);

unit test may be like this:

  @Test
  void testMyFix() {
    assertThat(catchThrowable(() ->ConfigUtil.getString("myfix","default"))).isNull();
  }

[jack-berg]

It would be nice to have a test recreating this CME and demonstrating that this fixes it. However, I spent a decent amount of time trying to hammer this with concurrent reads and writes and was unable to recreate it, so it could be specific to some version of java or the jdk.

I'm content give this a shot even without strong proof that it resolves it.

[laurit]

Looks like it does not reproduces on jdk 11+ probably because of https://github.com/openjdk/jdk11u/blob/cee8535a9d3de8558b4b5028d68e397e508bef71/src/java.base/share/classes/java/util/Properties.java#L159-L165
On jdk8 it reproduces with

import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;

public class Main {
    public static void main(String[] args) {
        int threads = 4;
        AtomicInteger counter = new AtomicInteger();
        Executor executor = Executors.newFixedThreadPool(threads);
        for (int i = 0; i < threads; i++) {
            executor.execute(new Runnable() {
                @Override
                public void run() {
                    while (true) {
                        String property = "prop " + counter.getAndIncrement();
                        System.setProperty(property, "a");
                        System.getProperties().remove(property);
                    }
                }
            });
        }

        while (true) {
            System.getProperties().entrySet().stream()
                    .filter(
                            entry -> "x".equals(entry.getKey().toString())
                    )
                    .map(entry -> entry.getValue().toString())
                    .findFirst()
                    .orElse(null);
        }
    }
}

ConcurrentModificationException does not reproduce with the proposed fix. It doesn't reproduce with

Properties properties = System.getProperties();
synchronized (properties) {
    properties.entrySet().stream()
            .filter(
                    entry -> "x".equals(entry.getKey().toString())
            )
            .map(entry -> entry.getValue().toString())
            .findFirst()
            .orElse(null);
}

either. Unlike the proposed fix this version does not make extra copies of system properties.

[jack-berg]

Good find @laurit!

I know there was some skepticism about taking a lock on System.getProperties(), but it seems ok to me. The work we're doing inside the lock is narrow and its hard for me to imagine how we could end up in a dead lock scenario.

[neugartf]

Anything missing here, @jack-berg ? Would be happy to see this in a release soon :).

[jack-berg]

Sorry for not getting back on this @neugartf. We talked about this in the 10/10/2024 java SIG, just before the 1.43.0 release. Decided it needed more work:

• Its not the only place we access system properties and iterate over them. DefaultConfigProperties also uses a similar pattern, and is likely more prone to the problem you report.
• We don't like the overhead of calling System.getProperties().clone(), and especially in DefaultConfigProperties where we couldn't call it once and cache the result like I proposed.
• And so we'd prefer to go with one of the other options, but since we can't reproduce the issue, we don't have confidence that the alternatives aren't subject to the same problem.
• What we really need is a reproduction. Based on the issue and my attempts to reproduce in a variety of java environments, I think this is limited to android. We don't have android tooling setup in this repository, but maybe you we can demonstrate in the opentelemetry-android project.

So in summary, to get this back on track, we need:

1. A reliable reproduction.
2. A fix we can show resolves the reproduction, preferably without cloning system properties.
3. A fix applied to both ConfigUtil and DefaultConfigProperties usage of system properties.

[neugartf]

Thanks for getting back (and with that detail)!

I'll invest some time to get this reproduced on the test app there (opentelemetry-android), though it won't be a proper junit test since we'll need to boot up the whole system + app.
I'll keep you posted!

[jack-berg]

I'll invest some time to get this reproduced on the test app there (opentelemetry-android), though it won't be a proper junit test since we'll need to boot up the whole system + app.

That would be great. If you can just show that you can read and write to System.getProperties() concurrently to recreate this, it would suffice. No need to jump through extra hoops to create the issue specifically with ConfigUtil.