OPENNLP-1883: Address review: concise contracts, shared thread routing, cache controls

The Stemmer and StemmerFactory interfaces state their contracts without
implementation or threading claims, and null words throw
IllegalArgumentException across the API (Snowball, factory products,
caching and sharing wrappers, Porter), pinned by tests. SnowballStemmer
now composes SharingStemmer so the per-thread routing pattern lives in
one class. CachingStemmer gains clearCache() for the calling thread,
interns cached stems through StringInterners, and both stateful classes
document the container release path of the *ME components.

Author: krickert

opennlp-api/src/main/java/opennlp/tools/stemmer/Stemmer.java

@@ -4,7 +4,7 @@
  * 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
+ * the License. You may obtain a copy of the License at
  *
  *     http://www.apache.org/licenses/LICENSE-2.0
  *
@@ -22,12 +22,7 @@
 /**
  * Reduces a word to its root form.
  *
- * <p>Thread safety is implementation-specific: check the implementation's documentation before
- * sharing an instance across threads. Stateful engines mutate internal buffers on each
- * {@link #stem(CharSequence)} call; when an implementation is not documented as thread-safe,
- * share a {@link StemmerFactory} across threads and confine each {@code Stemmer} to one thread,
- * or wrap the factory in a thread-local adapter when a single {@code Stemmer} reference must be
- * shared.</p>
+ * <p>Thread safety is implementation specific.</p>
  */
 public interface Stemmer {
 
@@ -36,16 +31,17 @@ public interface Stemmer {
    *
    * @param word The input word. Must not be {@code null}.
    * @return The stemmed form.
+   * @throws IllegalArgumentException Thrown if {@code word} is {@code null}.
    */
   CharSequence stem(CharSequence word);
 
   /**
-   * {@return every stem form for {@code word}} The default returns a single-element list with
-   * {@link #stem(CharSequence)}. Dictionary-based engines that can produce several root forms for
-   * one word override this; wrappers that delegate to another {@code Stemmer} must forward this
-   * method so a multi-output delegate keeps its full result list.
+   * {@return all stem forms for {@code word}} Defaults to a single-element list from
+   * {@link #stem(CharSequence)}. Dictionary-based engines may override this to return
+   * multiple roots; delegating wrappers must forward it to preserve the full list.
    *
    * @param word The input word. Must not be {@code null}.
+   * @throws IllegalArgumentException Thrown if {@code word} is {@code null}.
    */
   default List<CharSequence> stemAll(CharSequence word) {
     return List.of(stem(word));

opennlp-api/src/main/java/opennlp/tools/stemmer/StemmerFactory.java

@@ -18,26 +18,18 @@
 package opennlp.tools.stemmer;
 
 /**
- * An immutable, thread-safe factory for {@link Stemmer} instances.
- *
- * <p>Stateful stemming engines hold per-call mutable buffers and must not be shared across
- * threads. A {@code StemmerFactory} captures the configuration (algorithm, repeat count,
- * dictionary path, ...) and mints a fresh {@link Stemmer} on each {@link #newStemmer()} call. The
- * factory itself is safe to share; confine each returned {@link Stemmer} to a single thread, or
- * route through a thread-local adapter when a component must be shared.</p>
+ * A factory for {@link Stemmer} instances: it captures a stemmer configuration (algorithm,
+ * repeat count, dictionary path, ...) once and mints configured stemmers on demand.
  *
  * <p>Despite the name, this is not one of the {@code BaseToolFactory}-based tool factories (such
  * as {@code LemmatizerFactory}) that are instantiated by name from a model manifest. It is a
- * plain functional supplier of configured {@link Stemmer} instances and takes no part in the
- * model-loading mechanism.</p>
- *
- * <p>Implementations must be immutable and thread-safe after construction.</p>
+ * plain supplier of configured {@link Stemmer} instances and takes no part in the model-loading
+ * mechanism.</p>
  */
 public interface StemmerFactory {
 
   /**
-   * {@return a new {@link Stemmer} confined to the calling thread} Implementations must return a
-   * distinct instance on each call when the underlying engine is stateful.
+   * {@return a new {@link Stemmer}}
    */
   Stemmer newStemmer();
 }

opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/CachingStemmer.java

@@ -23,6 +23,7 @@
 
 import opennlp.tools.commons.ThreadSafe;
 import opennlp.tools.util.OwnerOrPerThreadState;
+import opennlp.tools.util.jvm.StringInterners;
 
 /**
  * A {@link Stemmer} that memoizes word-to-stem mappings in a bounded per-thread LRU cache.
@@ -40,8 +41,10 @@
  * <p>Because the cache is keyed to the thread, the memoization pays off on threads that are
  * reused across many words, such as a fixed platform-thread pool. On a virtual-thread-per-task
  * executor every task starts with an empty cache, so repeats are only served within one task.
- * Call {@link #clearThreadLocalState()} before returning a pooled thread that should not retain
- * its delegate and cache.</p>
+ * As with the thread-safe {@code *ME} components, long-running environments such as application
+ * containers should call {@link #clearThreadLocalState()} when a pooled thread no longer uses
+ * this stemmer; otherwise the thread retains its delegate and cache until the instance is
+ * unreachable, which can pin the defining classloader on redeploys.</p>
  *
  * <p>{@link #stemAll(CharSequence)} is forwarded to the delegate uncached, so multi-output
  * engines keep their full result list.</p>
@@ -89,14 +92,18 @@ public CachingStemmer(StemmerFactory factory, int capacity) {
 
   @Override
   public CharSequence stem(CharSequence word) {
+    if (word == null) {
+      throw new IllegalArgumentException("word must not be null");
+    }
     final ThreadState ts = state.get();
     final String key = word.toString();
     final String cached = ts.cache.get(key);
     if (cached != null) {
       return cached;
     }
-    // toString() detaches the result from any buffer the delegate may reuse internally.
-    final String stemmed = ts.delegate.stem(key).toString();
+    // toString() detaches the result from any buffer the delegate may reuse internally, and
+    // interning stores one instance per distinct stem across all threads' caches.
+    final String stemmed = StringInterners.intern(ts.delegate.stem(key).toString());
     ts.cache.put(key, stemmed);
     return stemmed;
   }
@@ -115,6 +122,14 @@ public void clearThreadLocalState() {
     state.clearForCurrentThread();
   }
 
+  /**
+   * Empties the calling thread's cache while keeping its delegate, forcing every subsequent
+   * word through a fresh stemming pass. Only the calling thread's cache is affected.
+   */
+  public void clearCache() {
+    state.get().cache.clear();
+  }
+
   private static final class ThreadState {
 
     private final Stemmer delegate;

opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/PorterStemmer.java

@@ -599,6 +599,9 @@ public String stem(String s) {
    * Returns the result as a CharSequence.
    */
   public CharSequence stem(CharSequence word) {
+    if (word == null) {
+      throw new IllegalArgumentException("word must not be null");
+    }
     return stem(word.toString());
   }
 

opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/snowball/SnowballStemmer.java

@@ -20,22 +20,26 @@
 import java.util.function.Supplier;
 
 import opennlp.tools.commons.ThreadSafe;
+import opennlp.tools.stemmer.SharingStemmer;
 import opennlp.tools.stemmer.Stemmer;
-import opennlp.tools.util.OwnerOrPerThreadState;
 
 /**
  * A {@link Stemmer} backed by the generated Snowball engines.
  *
  * <p>The generated engines hold mutable per-call buffers, so this class routes each call to a
- * per-thread engine via {@link OwnerOrPerThreadState}, the same pattern the thread-safe {@code *ME}
- * components use. A single {@code SnowballStemmer} instance is therefore safe to share across
- * threads.</p>
+ * per-thread engine, the same pattern the thread-safe {@code *ME} components use. A single
+ * {@code SnowballStemmer} instance is therefore safe to share across threads.</p>
+ *
+ * <p>As with the {@code *ME} components, long-running environments such as application
+ * containers should call {@link #clearThreadLocalState()} when a pooled thread no longer uses
+ * this stemmer; otherwise the thread retains its engine until the instance is unreachable,
+ * which can pin the defining classloader on redeploys.</p>
  */
 @ThreadSafe
 public class SnowballStemmer implements Stemmer {
 
-  private final OwnerOrPerThreadState<AbstractSnowballStemmer> delegates;
-  private final int repeat;
+  // All per-thread routing is delegated, so the pattern lives in exactly one class.
+  private final SharingStemmer sharing;
 
   /**
    * Creates a stemmer for the given algorithm and repeat count.
@@ -46,15 +50,7 @@ public class SnowballStemmer implements Stemmer {
    *     {@code repeat} is not positive.
    */
   public SnowballStemmer(ALGORITHM algorithm, int repeat) {
-    if (algorithm == null) {
-      throw new IllegalArgumentException("algorithm must not be null");
-    }
-    if (repeat <= 0) {
-      throw new IllegalArgumentException("repeat must be positive, got " + repeat);
-    }
-    this.repeat = repeat;
-    final Supplier<AbstractSnowballStemmer> engine = engineFor(algorithm);
-    this.delegates = new OwnerOrPerThreadState<>(engine, stemmer -> { });
+    this.sharing = new SharingStemmer(new SnowballStemmerFactory(algorithm, repeat));
   }
 
   /**
@@ -95,15 +91,7 @@ static Supplier<AbstractSnowballStemmer> engineFor(ALGORITHM algorithm) {
 
   @Override
   public CharSequence stem(CharSequence word) {
-    final AbstractSnowballStemmer stemmer = delegates.get();
-
-    stemmer.setCurrent(word.toString());
-
-    for (int i = 0; i < repeat; i++) {
-      stemmer.stem();
-    }
-
-    return stemmer.getCurrent();
+    return sharing.stem(word);
   }
 
   /**
@@ -112,7 +100,7 @@ public CharSequence stem(CharSequence word) {
    * {@code clearThreadLocalState()} on the thread-safe {@code *ME} components.
    */
   public void clearThreadLocalState() {
-    delegates.clearForCurrentThread();
+    sharing.clearThreadLocalState();
   }
 
   public enum ALGORITHM {

opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/snowball/SnowballStemmerFactory.java

@@ -25,11 +25,9 @@
  * A thread-safe factory that captures a Snowball stemmer configuration for APIs that accept a
  * {@link StemmerFactory}.
  *
- * <p>{@link #newStemmer()} returns a thread-confined stemmer that drives its generated engine
- * through a plain field, with none of the per-call thread routing of the shareable
- * {@link SnowballStemmer}. This keeps the one-stemmer-per-thread pattern (and per-thread
- * delegates inside caching or sharing wrappers) free of that indirection; use
- * {@link SnowballStemmer} directly when a single shared instance is wanted instead.</p>
+ * <p>Use this factory for the classic one-stemmer-per-thread pattern: {@link #newStemmer()}
+ * returns a plain, thread-confined stemmer without the per-call thread routing of the
+ * shareable {@link SnowballStemmer}.</p>
  *
  * @param algorithm The Snowball algorithm. Must not be {@code null}.
  * @param repeat    How many times to apply the stemmer per word; must be positive.
@@ -81,6 +79,9 @@ private ConfinedStemmer(AbstractSnowballStemmer engine, int repeat) {
 
     @Override
     public CharSequence stem(CharSequence word) {
+      if (word == null) {
+        throw new IllegalArgumentException("word must not be null");
+      }
       engine.setCurrent(word.toString());
       for (int i = 0; i < repeat; i++) {
         engine.stem();

opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/stemmer/StemmerFactoryTest.java

@@ -24,6 +24,7 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -328,4 +329,39 @@ void clearThreadLocalStateReleasesAWorkerThreadsDelegate() throws Exception {
     snowball.clearThreadLocalState();
     Assertions.assertEquals("run", snowball.stem("running").toString());
   }
+
+  @Test
+  void nullWordThrowsIllegalArgumentAcrossTheStemmerApi() {
+    final SnowballStemmerFactory factory =
+        new SnowballStemmerFactory(SnowballStemmer.ALGORITHM.ENGLISH);
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> new SnowballStemmer(SnowballStemmer.ALGORITHM.ENGLISH).stem(null));
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> factory.newStemmer().stem(null));
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> new CachingStemmer(factory).stem(null));
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> new SharingStemmer(factory).stem(null));
+    Assertions.assertThrows(IllegalArgumentException.class,
+        () -> new PorterStemmer().stem((CharSequence) null));
+  }
+
+  @Test
+  void clearCacheForcesRestemmingOnTheCallingThread() {
+    final AtomicInteger delegateCalls = new AtomicInteger();
+    final StemmerFactory counting = () -> word -> {
+      delegateCalls.incrementAndGet();
+      return word.toString();
+    };
+    final CachingStemmer cached = new CachingStemmer(counting);
+
+    cached.stem("running");
+    cached.stem("running");
+    Assertions.assertEquals(1, delegateCalls.get(), "the repeat is served from the cache");
+
+    cached.clearCache();
+    cached.stem("running");
+    Assertions.assertEquals(2, delegateCalls.get(),
+        "after clearCache() the word goes through the delegate again");
+  }
 }