search

unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

Static piCache hash map is not thread-safe #371

Closed stefanuhrig closed 2 years ago

stefanuhrig commented 2 years ago

Hi all,

If you have two threads and each thread has its own local UnitConverter instance, a concurrent call of #convert() can lead to a ConcurrentModificationException because access to the static Calculus$Pi#piCache variable is not synchronized: https://github.com/unitsofmeasurement/indriya/blob/b8e58ca04913c858038217e7c32bc9d26798518f/src/main/java/tech/units/indriya/function/Calculus.java#L150

This is a stack trace showing the issue:

Caused by: java.util.ConcurrentModificationException
    at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1135)
    at tech.units.indriya.function.Calculus$Pi.ofNumDigits(Calculus.java:150)
    at tech.units.indriya.function.PowerOfPiConverter.getValue(PowerOfPiConverter.java:125)
    at tech.units.indriya.function.PowerOfPiConverter.getValue(PowerOfPiConverter.java:48)
    at tech.units.indriya.function.MultiplyConverter.getFactor(MultiplyConverter.java:242)
    at tech.units.indriya.function.PowerOfPiConverter.convertWhenNotIdentity(PowerOfPiConverter.java:100)
    at tech.units.indriya.function.AbstractConverter$Pair.convertWhenNotIdentity(AbstractConverter.java:398)
    at tech.units.indriya.function.AbstractConverter.convert(AbstractConverter.java:221)

If you have further questions on the issue, please reach out.

Cheers, Stefan

keilw commented 2 years ago

Any suggestion or PR? @andi-huber, thoughts on that or an easy way to improve?

stefanuhrig commented 2 years ago

Well, a quick-fix would be switching from HashMap to Hashtable because Hashtable is synchronized by default. I can prepare a PR if you like. However, I'm not very familiar with the project, and there might be other occurrences of static unsynchronized cache variables.

keilw commented 2 years ago

While the JDK may never seriously remove Hashtable, I guess a slightly cleaner approach could be one of those: https://crunchify.com/hashmap-vs-concurrenthashmap-vs-synchronizedmap-how-a-hashmap-can-be-synchronized-in-java/

stefanuhrig commented 2 years ago

ConcurrentHashMap seems like a good choice to me because read operations won't block.

stefanuhrig commented 2 years ago

I've opened PR #372 for the issue.

andi-huber commented 2 years ago

I have merged a fix for Pi.

stefanuhrig commented 2 years ago

Thanks for fixing this so quickly!