rzo1 opened a new issue, #1506:
URL: https://github.com/apache/opennlp/issues/1506
`AbstractClassPathModelFinder.findModels(boolean)` is not thread-safe. Since
`DefaultClassPathModelProvider` delegates every `load(..)` call to it, loading
models via a shared `ClassPathModelProvider` from several threads can fail.
Affected: 3.0.0-M6 and current `main`
(`opennlp-core/opennlp-model-resolver`).
```java
if (this.models == null || reloadCache) {
final List<URI> classpathModels = getMatchingURIs("*.bin", getContext());
final List<URI> classPathProperties = getMatchingURIs("model.properties",
getContext());
this.models = new HashSet<>(); // published before it is filled
for (URI model : classpathModels) {
...
this.models.add(new ClassPathModelEntry(model, Optional.ofNullable(m)));
}
}
return this.models;
```
Problems:
1. The (non-volatile) field is assigned an empty `HashSet` before it is
populated. A second thread calling `findModels(false)` meanwhile sees `models
!= null` and gets an empty or partially filled set, so
`ClassPathModelLoader.load(..)` does not find a model that is actually on the
classpath.
2. The returned set is the live, mutable cache. A concurrent first call or
`reloadCache = true` adds to a `HashSet` while another thread iterates over it
in the loader, which can throw `ConcurrentModificationException` or miss
entries.
3. Two threads that both see `models == null` scan the classpath twice and
overwrite each other's set.
We hit this in an application server where a shared provider loads different
models (sentence detector, tokenizer, POS tagger) lazily from concurrent
requests: the first parallel loads randomly failed or found no model. As a
workaround we serialize all calls to the provider with an external lock.
Suggested fix: build the set in a local variable and publish it only once it
is complete, as an unmodifiable set via a `volatile` field (or an
`AtomicReference`). Guard the scan with a lock, or use double-checked locking,
so the classpath is scanned only once for concurrent first calls. Also document
the thread-safety of `ClassPathModelFinder` and `ClassPathModelProvider` in
their Javadoc. A test could start N threads behind a `CountDownLatch` and call
`findModels(false)` on a fresh finder, asserting that every caller gets the
complete set.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
