Open neugartf opened 1 week ago
ConfigUtil.getString()
uses System.getProperties()
and Properties
is not thread safe. Is it possible that your application in some other thread at the same time may be changing System.Properties
in any way ?
I may be wrong, but to me it seems that making java.util.Properties
thread safe is the only way.
at io.opentelemetry.sdk.metrics.internal.debug.DebugConfig.
(DebugConfig.java:25)
This is unusual because its in the static initialization block of DebugConfig, which is only called once at startup.
We could do something like new HashMap<>(System.getProperties())
to make a copy of the system properties, but even that iterates over the the entries of the properties and may be susceptible (somebody fact check me on this) to concurrent modification exceptions.
We could also wrap the iteration with in a try / catch.
We could also find a way to iterate over the keys in a way that avoid concurrent modification exceptions. According to the HashTable
javadoc:
The Enumerations returned by Hashtable's keys and elements methods are not fail-fast.
This suggests that something like System.getProperties().keys()
could be used in iteration in the ConfigUtil#getString
method. Would want to study / test more to be sure.
@jack-berg how about using ConcurrentHashMap to store a copy of the properties for iteration?
also AFAIK static block runs once when the class is loaded. I suppose DebugConfig.isMetricsDebugEnabled()
in SourceInfo.fromCurrentStack()
is the first time DebugConfig
is referenced and hence loaded at that time. Maybe if we reference it earlier, but i don't think that would solve the issue.
Is it possible that your application in some other thread at the same time may be changing System.Properties in any way ?
@harshitrjpt : Theoretically no, but honestly Android might be doing some things here we don't have really insight about.
Would also vouch for a copy
System.getProperties().entrySet().stream().toArray()...
// or
Set.copyOf(properties.entrySet())
That should make iterating safe but ofc we wouldn't know about any changes to the list. Don't know if we necessarily need to.
That should make iterating safe but ofc we wouldn't know about any changes to the list. Don't know if we necessarily need to.
It seems like we have no choice but to accept the possibility of changes between when we read the keyset and read the values.
Describe the bug There is a
ConcurrentModificationException
thrown while callingbuild()
onLongGaugeBuilder
.Steps to reproduce not yet
What did you expect to see? No exception thrown
What did you see instead?
ConcurrentModificationException
What version and what artifacts are you using? Artifacts: see
*.gradle.kts
excerpt below Version: 1.42.1 How did you reference these artifacts?Environment Compiler: JDK 17 - Temurin OS: Ubuntu (unknown version) Runtime (if different from JDK above): Android runtime (ART) OS (if different from OS compiled on): Android
Additional context