RFR: 8341964: Add mechanism to disable different parts of TLS cipher suite [v4]

[Sean Mullan]

On Thu, 7 Nov 2024 20:09:59 GMT, Artur Barashev <abarashev at 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