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]

If system properties are modified I'd guess you could still get a concurrent modification error from the hash set constructor. Perhaps this class should create normalized copies of system properties and env in static initializer.

[neugartf]

You are right! I modified the commit a bit in order to access the immutable set of property names that Properties provides. Let me know what you think! :)

[trask]

I think you could apply the filter directly on the stringPropertyNames (without creating the temporary map)

[neugartf]

You are completely right! Just changed it. Hope that's what you meant 🙃 .

[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]

Couldn't reproduce the ConcurrentModificationException on jdk 8, 11 nor 17 in a scratch file, so possibly keys() also isn't safe.
I'd suggest to create a clone() which is marked as synchronized in jdk 8 and 11.

[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)

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);

[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();
  }