On Thu, 7 Nov 2024 20:09:59 GMT, Artur Barashev <abaras...@openjdk.org> wrote:
>> src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
>> line 126:
>>
>>> 124: }
>>> 125:
>>> 126: Pattern p = patternCache.get(pattern);
>>
>> I think you want to use `putIfAbsent` here, so the operation happens
>> atomically.
>
> Actually if we do `patternCache.putIfAbsent(pattern,
> Pattern.compile(pattern.replace("*", ".*")))` we'll be computing Pattern on
> every call regardless if it's already present in cache. Also `putIfAbsent`
> makes no guarantees of atomicity. Better candidate here seems to be
> `computeIfAbsent` method, but still it would not be as fast as the current
> solution because it would add an extra `get` call to the cache interaction.
> That's how exactly `computeIfAbsent` method works:
>
> ` * <pre> {@code
> * if (map.get(key) == null) {
> * V newValue = mappingFunction.apply(key);
> * if (newValue != null)
> * map.put(key, newValue);
> * }
> * }</pre>
> `
Good point, `computeIfAbsent` is what you would want. Both `putIfAbsent` and
`computeIfAbsent` are done atomically though (the javadoc states that).
I think it boils down to whether the existing code is ok and would not cause
any unexpected behavior. AFAICT, there is a small chance for a race condition
where more than one thread could end up computing and storing the same Pattern,
but it should be the same algorithm, so there should be no negative side
effects.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21841#discussion_r1833331019
