Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ConcurrentModificationException #6732 #6746

Closed
wants to merge 5 commits into from
Closed

Fix ConcurrentModificationException #6732 #6746

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab84470
Fix ConcurrentModificationException #6732
neugartf Sep 25, 2024
b115c5a
Move to stringPropertyNames in order to iterate on a stable list.
neugartf Sep 26, 2024
af48863
Improve stream handling of system property
neugartf Sep 26, 2024
329c05d
Merge branch 'open-telemetry:main' into main
neugartf Sep 26, 2024
d736406
Cloning properties in order to safely iterate over them
neugartf Sep 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import javax.annotation.Nullable;

/**
Expand All @@ -33,10 +34,12 @@ private ConfigUtil() {}
*/
public static String getString(String key, String defaultValue) {
String normalizedKey = normalizePropertyKey(key);

String systemProperty =
neugartf marked this conversation as resolved.
Show resolved Hide resolved
System.getProperties().entrySet().stream()
.filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString())))
.map(entry -> entry.getValue().toString())
System.getProperties().stringPropertyNames().stream()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@neugartf neugartf Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In jdk11 that method is not synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

??

.filter(propertyName -> normalizedKey.equals(normalizePropertyKey(propertyName)))
.map(System::getProperty)
.filter(Objects::nonNull)
.findFirst()
.orElse(null);
if (systemProperty != null) {
Expand Down
Loading