Fix ConfigUtil#getString ConcurrentModificationException #6841
Conversation
|
|
||
| @Test | ||
| @SuppressWarnings("ReturnValueIgnored") | ||
| void systemPropertiesConcurrentAccess() throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
This test fails on java 8 if you call System.getProperties() instead of ConfigUtil#safeSystemProperties().
| * of exception. See https://github.com/open-telemetry/opentelemetry-java/issues/6732 for details. | ||
| */ | ||
| public static Properties safeSystemProperties() { | ||
| return (Properties) System.getProperties().clone(); |
There was a problem hiding this comment.
Cloning seems to be the simplest way to accommodate various use cases. Anything other option that returns Properties ends up mimicking the logic of clone() anyway. If clone() ends up being too expensive (I doubt it), we can cache a copy.
There was a problem hiding this comment.
I'm ok with it
ConfigUtil.getString(..) is already slow (too slow to be used on the hotpath) since it normalizes all the entry keys on each call
There was a problem hiding this comment.
If clone() ends up being too expensive
clone is only needed on jdk8 and android so we could skip it on other jdks if we so wish. Untested code follows
private static final CLONE_PROPERTIES = System.getProperty("java.version").equals("0") // android
|| System.getProperty("java.specification.version").startsWith("1."); // jdk8There was a problem hiding this comment.
clone is only needed on jdk8 and android so we could skip it on other jdks if we so wish.
That's true. I'd prefer to keep that in our back pocket in case we need it. As a general rule, I think we avoid runtime-specific code unless we have a strong reason (subjective definition of "strong").
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6841 +/- ##
============================================
+ Coverage 90.08% 90.48% +0.40%
- Complexity 6464 6598 +134
============================================
Files 718 731 +13
Lines 19528 19737 +209
Branches 1923 1938 +15
============================================
+ Hits 17591 17859 +268
+ Misses 1348 1285 -63
- Partials 589 593 +4 ☔ View full report in Codecov by Sentry. |
| * of exception. See https://github.com/open-telemetry/opentelemetry-java/issues/6732 for details. | ||
| */ | ||
| public static Properties safeSystemProperties() { | ||
| return (Properties) System.getProperties().clone(); |
There was a problem hiding this comment.
I'm ok with it
ConfigUtil.getString(..) is already slow (too slow to be used on the hotpath) since it normalizes all the entry keys on each call
Resolves #6732.
Builds on #6746.