[I] Race condition on cacheModified flag in ToolchainDiscoverer from parallelStream() [maven-toolchains-plugin]

["elharo (via GitHub)" <gi...@apache.org>]

elharo opened a new issue, #173:
URL: https://github.com/apache/maven-toolchains-plugin/issues/173

   ## Summary
   
   `ToolchainDiscoverer.getToolchainModel()` is called from a `parallelStream()` in `discoverToolchains()`. The `cacheModified` volatile field is written concurrently by multiple threads without atomic coordination, which can cause cache updates to be lost.
   
   ## Location
   
   `ToolchainDiscoverer.java:217-227`
   
   https://github.com/apache/maven-toolchains-plugin/blob/master/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java#L217-L227
   
   ## Code
   
   ```java
   ToolchainModel getToolchainModel(Path jdk) {
       ToolchainModel model = cache.get(jdk);
       if (model == null) {
           model = doGetToolchainModel(jdk);
           if (model != null) {
               cache.put(jdk, model);
               cacheModified = true;
           }
       }
       return model;
   }
   ```
   
   Called from `discoverToolchains()` via parallel stream:
   
   ```java
   List<ToolchainModel> tcs = jdks.parallelStream()
           .map(s -> {
               ToolchainModel tc = getToolchainModel(s);
               // ...
           })
           .sorted(...)
           .collect(Collectors.toList());
   writeCache();
   ```
   
   ## Problem
   
   1. `cacheModified = true` is written from multiple threads in the parallel stream without synchronization
   2. After the parallel stream completes, `writeCache()` reads `cacheModified`, writes the cache file, and sets it back to `false`
   3. If a thread writes `cacheModified = true` after `writeCache()` has read it but before it sets it to `false`, the cache update is silently lost, and the newly discovered JDK won't be persisted
   4. The `ConcurrentHashMap` itself is fine (thread-safe), but the coordination between `cacheModified` and the cache file write is not atomic
   
   ## Impact
   
   Under concurrent load (which is the default for `parallelStream()`), some discovered JDK toolchains may not be persisted to the cache file. This means `writeCache()` may be called with `cacheModified == false` even though new entries were added during the stream.
   
   ## Suggested Fix
   
   Use an `AtomicBoolean` for `cacheModified` or re-check cache for new entries at write time:
   
   ```java
   private final AtomicBoolean cacheModified = new AtomicBoolean();
   
   // In getToolchainModel:
   cacheModified.set(true);
   
   // In writeCache:
   cacheModified.set(false);
   ```
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Race condition on cacheModified flag in ToolchainDiscoverer from parallelStream() [maven-toolchains-plugin]

["efegokdemir (via GitHub)" <gi...@apache.org>]

efegokdemir commented on issue #173:
URL: https://github.com/apache/maven-toolchains-plugin/issues/173#issuecomment-6064636055

   I traced the race to overlapping calls on the singleton discoverer: a cache entry can be added while another invocation is persisting, then the writer clears the dirty flag. I’ll add a deterministic regression for that overlap and make cache updates coordinate with persistence.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org