This is an automated email from the ASF dual-hosted git repository.
rzo1 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/opennlp.git
The following commit(s) were added to refs/heads/main by this push:
new 0e95db1ce GH-1506: Make AbstractClassPathModelFinder.findModels
thread-safe (#1507)
0e95db1ce is described below
commit 0e95db1ce38d2c4e220d9beb4e8ed461d70e4d9b
Author: Richard Zowalla <[email protected]>
AuthorDate: Thu Oct 22 09:53:56 2026 +0200
GH-1506: Make AbstractClassPathModelFinder.findModels thread-safe (#1507)
The model cache was published before it was filled, and callers got the
mutable cache itself. Concurrent first calls of a shared finder or
DefaultClassPathModelProvider could therefore see no models, scan the
classpath several times, or fail with a ConcurrentModificationException.
findModels now builds the set locally and publishes it as an unmodifiable
set through a volatile field, and a lock makes concurrent first calls scan
once. The Javadoc of the finder and provider types states their thread
safety.
---
README.md | 1 +
.../opennlp/tools/models/ClassPathModelFinder.java | 2 +
.../tools/models/AbstractClassPathModelFinder.java | 63 ++++--
.../tools/models/ClassPathModelProvider.java | 2 +
.../models/DefaultClassPathModelProvider.java | 3 +
...bstractClassPathModelFinderConcurrencyTest.java | 212 +++++++++++++++++++++
6 files changed, 267 insertions(+), 16 deletions(-)
diff --git a/README.md b/README.md
index 42ec642ec..0a9f83f2c 100644
--- a/README.md
+++ b/README.md
@@ -142,6 +142,7 @@ The list below covers the changes most likely to affect
existing code. It is not
- `PlainTextByLineStream.reset()` closes the previous reader, so its
`InputStreamFactory` must return a new stream for each pass.
- `RealBasicEventStream`: a line that contains only an outcome is read as an
event without contexts and no longer ends the stream.
- Parser: `ParserFactory.create` and the chunking and tree insert `Parser`
constructors throw `IllegalArgumentException` for a beam size below 1 or an
advance percentage outside (0, 1]
([GH-1476](https://github.com/apache/opennlp/issues/1476)).
+- Model resolver: `AbstractClassPathModelFinder.findModels` returns an
unmodifiable set, so the classpath finders and `DefaultClassPathModelProvider`
can be shared between threads
([GH-1506](https://github.com/apache/opennlp/issues/1506)).
### Thread safety
diff --git
a/opennlp-api/src/main/java/opennlp/tools/models/ClassPathModelFinder.java
b/opennlp-api/src/main/java/opennlp/tools/models/ClassPathModelFinder.java
index 9e502e4ae..5139db18e 100644
--- a/opennlp-api/src/main/java/opennlp/tools/models/ClassPathModelFinder.java
+++ b/opennlp-api/src/main/java/opennlp/tools/models/ClassPathModelFinder.java
@@ -22,6 +22,8 @@ import java.util.Set;
* Describes a scanner which detects OpenNLP specific model files in an
applications's classpath.
* If compatible models are present at runtime, each discovered element is
made available
* as {@link ClassPathModelEntry}.
+ * <p>
+ * Thread safety is implementation specific.
*/
public interface ClassPathModelFinder {
diff --git
a/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/AbstractClassPathModelFinder.java
b/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/AbstractClassPathModelFinder.java
index 2409c1823..d31f2b5f4 100644
---
a/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/AbstractClassPathModelFinder.java
+++
b/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/AbstractClassPathModelFinder.java
@@ -22,6 +22,7 @@ import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.Enumeration;
import java.util.HashSet;
import java.util.List;
@@ -34,6 +35,7 @@ import java.util.jar.JarFile;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import opennlp.tools.commons.ThreadSafe;
import opennlp.tools.util.ParamChecks;
/**
@@ -52,9 +54,15 @@ import opennlp.tools.util.ParamChecks;
* including spaces and non-ASCII characters. Other subclasses may apply their
own mask rules:
* {@link opennlp.tools.models.classgraph.ClassgraphModelFinder} follows the
wildcard rules of
* the ClassGraph library.
+ * <p>
+ * {@link #findModels(boolean)} may be called from several threads. Concurrent
first calls scan
+ * the classpath once, and every caller receives the complete, unmodifiable
set of models.
+ * Subclasses must keep {@link #getContext()} and {@link
#getMatchingURIs(String, Object)} free
+ * of shared mutable state.
*
* @see ClassPathModelFinder
*/
+@ThreadSafe
public abstract class AbstractClassPathModelFinder implements
ClassPathModelFinder {
protected static final String JAR = "jar";
@@ -66,7 +74,8 @@ public abstract class AbstractClassPathModelFinder implements
ClassPathModelFind
private static final String URL_MUST_NOT_BE_NULL = "url must not be null";
private final String jarModelPrefix;
- private Set<ClassPathModelEntry> models;
+ private final Object scanLock = new Object();
+ private volatile Set<ClassPathModelEntry> models;
/**
* By default, it scans for {@link #OPENNLP_MODEL_JAR_PREFIX}.
@@ -84,28 +93,50 @@ public abstract class AbstractClassPathModelFinder
implements ClassPathModelFind
this.jarModelPrefix = ParamChecks.requireNonNullArg(jarModelPrefix,
"jarModelPrefix");
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>The result is cached; concurrent first calls scan the classpath once.
The returned set
+ * is unmodifiable and does not change when the cache is reloaded.</p>
+ */
@Override
public Set<ClassPathModelEntry> findModels(boolean reloadCache) {
+ Set<ClassPathModelEntry> found = this.models;
+ if (found != null && !reloadCache) {
+ return found;
+ }
+ synchronized (scanLock) {
+ found = this.models;
+ if (found == null || reloadCache) {
+ found = scanModels();
+ this.models = found;
+ }
+ return found;
+ }
+ }
- if (this.models == null || reloadCache) {
- final List<URI> classpathModels = getMatchingURIs("*.bin", getContext());
- final List<URI> classPathProperties =
getMatchingURIs("model.properties", getContext());
-
- this.models = new HashSet<>();
+ /**
+ * Scans the classpath for models and pairs each model with the {@code
model.properties} of
+ * its jar.
+ *
+ * @return The unmodifiable set of model entries. It may be empty.
+ */
+ private Set<ClassPathModelEntry> scanModels() {
+ final List<URI> classpathModels = getMatchingURIs("*.bin", getContext());
+ final List<URI> classPathProperties = getMatchingURIs("model.properties",
getContext());
- for (URI model : classpathModels) {
- URI m = null;
- for (URI prop : classPathProperties) {
- if (jarPathsMatch(model, prop)) {
- m = prop;
- break;
- }
+ final Set<ClassPathModelEntry> found = new HashSet<>();
+ for (URI model : classpathModels) {
+ URI m = null;
+ for (URI prop : classPathProperties) {
+ if (jarPathsMatch(model, prop)) {
+ m = prop;
+ break;
}
- this.models.add(new ClassPathModelEntry(model,
Optional.ofNullable(m)));
-
}
+ found.add(new ClassPathModelEntry(model, Optional.ofNullable(m)));
}
- return this.models;
+ return Collections.unmodifiableSet(found);
}
/**
diff --git
a/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/ClassPathModelProvider.java
b/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/ClassPathModelProvider.java
index f21b86ea3..02679f8e8 100644
---
a/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/ClassPathModelProvider.java
+++
b/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/ClassPathModelProvider.java
@@ -28,6 +28,8 @@ import opennlp.tools.util.model.BaseModel;
* Providing models available in the classpath requires a {@link
ClassPathModelFinder finder}
* and a {@link ClassPathModelLoader} at runtime. Depending on the environment,
* a custom {@code finder} can be used.
+ * <p>
+ * Thread safety is implementation specific.
*
* @see BaseModel
* @see ClassPathModelFinder
diff --git
a/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/DefaultClassPathModelProvider.java
b/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/DefaultClassPathModelProvider.java
index ee923e6cf..21de00155 100644
---
a/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/DefaultClassPathModelProvider.java
+++
b/opennlp-core/opennlp-model-resolver/src/main/java/opennlp/tools/models/DefaultClassPathModelProvider.java
@@ -25,6 +25,9 @@ import opennlp.tools.util.model.BaseModel;
/**
* A default implementation of {@link ClassPathModelProvider} which by default
relies on
* {@link SimpleClassPathModelFinder} to scan for models in the current
classpath.
+ * <p>
+ * An instance can be shared between threads if its {@link
ClassPathModelFinder} is thread-safe,
+ * as the finders based on {@link AbstractClassPathModelFinder} are.
*
* @see ClassPathModelFinder
* @see ClassPathModelLoader
diff --git
a/opennlp-core/opennlp-model-resolver/src/test/java/opennlp/tools/models/AbstractClassPathModelFinderConcurrencyTest.java
b/opennlp-core/opennlp-model-resolver/src/test/java/opennlp/tools/models/AbstractClassPathModelFinderConcurrencyTest.java
new file mode 100644
index 000000000..a12584e7f
--- /dev/null
+++
b/opennlp-core/opennlp-model-resolver/src/test/java/opennlp/tools/models/AbstractClassPathModelFinderConcurrencyTest.java
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package opennlp.tools.models;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests that {@link AbstractClassPathModelFinder#findModels(boolean)} can be
called from several
+ * threads.
+ */
+class AbstractClassPathModelFinderConcurrencyTest {
+
+ private static final int THREADS = 8;
+
+ private static final long TIMEOUT_SECONDS = 10;
+
+ private static final List<URI> MODELS = List.of(
+
URI.create("jar:file:/repo/opennlp-models-sentdetect-en.jar!/en-sent.bin"),
+
URI.create("jar:file:/repo/opennlp-models-tokenizer-en.jar!/en-token.bin"),
+ URI.create("jar:file:/repo/opennlp-models-pos-en.jar!/en-pos.bin"));
+
+ private static final URI OTHER_MODEL =
+
URI.create("jar:file:/repo/opennlp-models-langdetect.jar!/langdetect.bin");
+
+ /**
+ * Concurrent first calls scan once, and every caller gets the complete set,
although all of
+ * them call while the first scan is still running.
+ */
+ @Test
+ void testConcurrentFirstCallsScanOnceAndReturnAllModels() throws
InterruptedException {
+ final CountDownLatch release = new CountDownLatch(1);
+ final ScanCountingFinder finder = new ScanCountingFinder(MODELS, release);
+ final List<Set<ClassPathModelEntry>> results = new ArrayList<>();
+ final List<Thread> threads = new ArrayList<>();
+ for (int i = 0; i < THREADS; i++) {
+ final Thread thread = new Thread(() -> {
+ final Set<ClassPathModelEntry> found = finder.findModels(false);
+ synchronized (results) {
+ results.add(found);
+ }
+ });
+ threads.add(thread);
+ thread.start();
+ }
+
+ awaitAllWaiting(threads);
+ release.countDown();
+ for (Thread thread : threads) {
+ thread.join(TimeUnit.SECONDS.toMillis(TIMEOUT_SECONDS));
+ }
+
+ assertEquals(1, finder.scans());
+ assertEquals(THREADS, results.size());
+ for (Set<ClassPathModelEntry> found : results) {
+ assertEquals(entries(MODELS), found);
+ }
+ }
+
+ /** The returned set is not the cache itself, so callers cannot change it. */
+ @Test
+ void testReturnedModelsAreUnmodifiable() {
+ final ScanCountingFinder finder = new ScanCountingFinder(MODELS,
released());
+ final Set<ClassPathModelEntry> found = finder.findModels(false);
+ final ClassPathModelEntry other = new ClassPathModelEntry(OTHER_MODEL,
Optional.empty());
+ assertThrows(UnsupportedOperationException.class, () -> found.add(other));
+ }
+
+ /** A reload scans again and replaces the cache; a set returned earlier
keeps its content. */
+ @Test
+ void testReloadReplacesTheCacheAndKeepsEarlierResults() {
+ final ScanCountingFinder finder = new ScanCountingFinder(MODELS,
released());
+ final Set<ClassPathModelEntry> first = finder.findModels(false);
+ assertSame(first, finder.findModels(false));
+
+ finder.setModels(List.of(OTHER_MODEL));
+ final Set<ClassPathModelEntry> reloaded = finder.findModels(true);
+
+ assertEquals(2, finder.scans());
+ assertEquals(entries(MODELS), first);
+ assertEquals(entries(List.of(OTHER_MODEL)), reloaded);
+ assertSame(reloaded, finder.findModels(false));
+ }
+
+ /**
+ * Waits until every thread waits, either for the scan to be released or for
the scan lock.
+ *
+ * @param threads The threads that call the finder.
+ * @throws InterruptedException Thrown if the test thread is interrupted.
+ */
+ private static void awaitAllWaiting(List<Thread> threads) throws
InterruptedException {
+ final long deadline = System.nanoTime() +
TimeUnit.SECONDS.toNanos(TIMEOUT_SECONDS);
+ for (Thread thread : threads) {
+ while (!isWaiting(thread.getState())) {
+ assertTrue(System.nanoTime() < deadline, thread.getName() + " did not
reach the finder");
+ Thread.sleep(1);
+ }
+ }
+ }
+
+ /**
+ * {@return whether a thread in {@code state} waits for a latch or a lock}
+ *
+ * @param state The state of a thread.
+ */
+ private static boolean isWaiting(Thread.State state) {
+ return state == Thread.State.WAITING || state == Thread.State.TIMED_WAITING
+ || state == Thread.State.BLOCKED;
+ }
+
+ /** {@return a latch that is already released} */
+ private static CountDownLatch released() {
+ return new CountDownLatch(0);
+ }
+
+ /**
+ * {@return the entries the finder creates for {@code models}, which have no
properties}
+ *
+ * @param models The model {@link URI URIs}.
+ */
+ private static Set<ClassPathModelEntry> entries(List<URI> models) {
+ final Set<ClassPathModelEntry> entries = new HashSet<>();
+ for (URI model : models) {
+ entries.add(new ClassPathModelEntry(model, Optional.empty()));
+ }
+ return entries;
+ }
+
+ /**
+ * A finder that counts its scans and holds each scan until a latch is
released.
+ */
+ private static final class ScanCountingFinder extends
AbstractClassPathModelFinder {
+
+ private final AtomicInteger scans = new AtomicInteger();
+ private final CountDownLatch release;
+ private volatile List<URI> models;
+
+ /**
+ * @param models The model {@link URI URIs} a scan finds.
+ * @param release The latch every scan waits for.
+ */
+ ScanCountingFinder(List<URI> models, CountDownLatch release) {
+ this.models = models;
+ this.release = release;
+ }
+
+ /**
+ * @param models The model {@link URI URIs} the next scan finds.
+ */
+ void setModels(List<URI> models) {
+ this.models = models;
+ }
+
+ /** {@return the number of scans so far} */
+ int scans() {
+ return scans.get();
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ protected Object getContext() {
+ return null;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ protected List<URI> getMatchingURIs(String wildcardPattern, Object
context) {
+ if (!"*.bin".equals(wildcardPattern)) {
+ return List.of();
+ }
+ scans.incrementAndGet();
+ try {
+ if (!release.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
+ throw new IllegalStateException("the scan was not released");
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IllegalStateException(e);
+ }
+ return models;
+ }
+ }
+}
