RFR: 8292983: ModuleReferenceImpl.computeHash should record algorithm for cache checks [v2]

[Alan Bateman]

On Fri, 26 Aug 2022 14:14:13 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Look at implementation and figure out what happens if you do:
>> 
>> 
>>  computeHash("SHA-1") = someHash;
>>  computeHash("SHA-256") = ...?
>> 
>> 
>> The caching method should actually check the algorithms match.
>> 
>> Not a bug at this point, since only use SHA-256 today, but this is a landmine ready to fire. 
>> 
>> Additional testing:
>>  - [x] Linux x86_64 release, `java/lang/module` tests
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Touchups

src/java.base/share/classes/jdk/internal/module/ModuleReferenceImpl.java line 70:

> 68:     // ConcurrentMap<String,byte[]>.
> 69:     //
> 70:     // For correctness under concurrent updates, we need to wrap the fields

The comment "If there is a significant ..." reads more like a "TODO" and can be removed.

src/java.base/share/classes/jdk/internal/module/ModuleReferenceImpl.java line 75:

> 73:         }
> 74:         final byte[] hash;
> 75:         final String algorithm;

Have you tried using a record? Also can you move the declaration of cachedHash to after the declaration of the record.

src/java.base/share/classes/jdk/internal/module/ModuleReferenceImpl.java line 154:

> 152:         CachedHash ch = cachedHash;
> 153:         if (ch != null) {
> 154:             if (ch.algorithm.equals(algorithm)) {

The nested if looks a bit strange here, you can change to:

if (ch != null & ch.algorithm.equals(algorithm))
    return ch.hash;

-------------

PR: https://git.openjdk.org/jdk/pull/10044