Issue #234: Using builtin annotation classes before creating a CAS can break type system management

uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java

@@ -388,6 +388,17 @@ static int clToType2JCasSize() {
     return cl_to_type2JCas.size();
   }
 
+  /**
+   * Test support: checks whether the given class loader is currently registered in the global JCas
+   * class cache. Unlike {@link #clToType2JCasSize()}, this is not affected by other (unrelated) class
+   * loaders being asynchronously reaped from the underlying weak map.
+   */
+  static boolean isClToType2JCasRegistered(ClassLoader cl) {
+    synchronized (cl_to_type2JCas) {
+      return cl_to_type2JCas.get(cl) != null;
+    }
+  }
+
   private static void loadBuiltins(TypeImpl ti, ClassLoader cl,
           Map<String, JCasClassInfo> type2jcci, ArrayList<MutableCallSite> callSites_toSync) {
     String typeName = ti.getName();

uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemConstants.java

@@ -101,23 +101,24 @@ public interface TypeSystemConstants {
   /**
    * adjOffsets for builtin Features
    */
-  int sofaNumFeatAdjOffset = TypeSystemImpl.staticTsi.sofaType
+  int sofaNumFeatAdjOffset = TypeSystemImpl.committedStaticTsi().sofaType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_SOFANUM);
-  int sofaIdFeatAdjOffset = TypeSystemImpl.staticTsi.sofaType
+  int sofaIdFeatAdjOffset = TypeSystemImpl.committedStaticTsi().sofaType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_SOFAID);
-  int sofaStringFeatAdjOffset = TypeSystemImpl.staticTsi.sofaType
+  int sofaStringFeatAdjOffset = TypeSystemImpl.committedStaticTsi().sofaType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_SOFASTRING);
-  int sofaMimeFeatAdjOffset = TypeSystemImpl.staticTsi.sofaType
+  int sofaMimeFeatAdjOffset = TypeSystemImpl.committedStaticTsi().sofaType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_SOFAMIME);
-  int sofaUriFeatAdjOffset = TypeSystemImpl.staticTsi.sofaType
+  int sofaUriFeatAdjOffset = TypeSystemImpl.committedStaticTsi().sofaType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_SOFAURI);
-  int sofaArrayFeatAdjOffset = TypeSystemImpl.staticTsi.sofaType
+  int sofaArrayFeatAdjOffset = TypeSystemImpl.committedStaticTsi().sofaType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_SOFAARRAY);
-  int annotBaseSofaFeatAdjOffset = TypeSystemImpl.staticTsi.annotBaseType
+  int annotBaseSofaFeatAdjOffset = TypeSystemImpl.committedStaticTsi().annotBaseType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_SOFA);
-  int beginFeatAdjOffset = TypeSystemImpl.staticTsi.annotType
+  int beginFeatAdjOffset = TypeSystemImpl.committedStaticTsi().annotType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_BEGIN);
-  int endFeatAdjOffset = TypeSystemImpl.staticTsi.annotType.getAdjOffset(CAS.FEATURE_BASE_NAME_END);
-  int langFeatAdjOffset = TypeSystemImpl.staticTsi.docType
+  int endFeatAdjOffset = TypeSystemImpl.committedStaticTsi().annotType
+          .getAdjOffset(CAS.FEATURE_BASE_NAME_END);
+  int langFeatAdjOffset = TypeSystemImpl.committedStaticTsi().docType
           .getAdjOffset(CAS.FEATURE_BASE_NAME_LANGUAGE);
 }

uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java

@@ -2867,11 +2867,31 @@ public List<TypeImpl> getAllTypes() {
   }
 
   public static final TypeSystemImpl staticTsi = new TypeSystemImpl();
-  static {
-    TypeSystemImpl tsi = staticTsi.commit(); // needed to assign adjusted offsets to the builtins
-    if (tsi != staticTsi) {
-      Misc.internalError();
+
+  // staticTsi.commit() can consolidate to a structurally-equivalent TypeSystemImpl that committed
+  // first, in which case staticTsi itself stays unfinalized. We cache and return whatever commit()
+  // returns -- that is the instance with the correctly computed offsets.
+  private static volatile TypeSystemImpl committedStaticTsi;
+
+  /**
+   * Returns a committed builtin-only {@link TypeSystemImpl} -- either {@link #staticTsi} itself if
+   * it was the first one to commit, or whatever structurally-equivalent committed instance it was
+   * consolidated to. Commit is performed lazily and exactly once across all threads. Code that
+   * needs commit-dependent state (e.g. {@link TypeImpl#getAdjOffset(String)}) must go through this
+   * accessor rather than reading {@link #staticTsi} directly.
+   */
+  public static TypeSystemImpl committedStaticTsi() {
+    TypeSystemImpl result = committedStaticTsi;
+    if (result == null) {
+      synchronized (TypeSystemImpl.class) {
+        result = committedStaticTsi;
+        if (result == null) {
+          result = staticTsi.commit();
+          committedStaticTsi = result;
+        }
+      }
     }
+    return result;
   }
 
 //@formatter:off
@@ -3106,20 +3126,22 @@ public static final MutableCallSite createCallSite(Class<? extends TOP> clazz, S
    */
   public static final MutableCallSite createCallSiteForBuiltIn(Class<? extends TOP> clazz,
           String featName) {
-    // If the static TSI has not yet been initialized, we assume that the initialization of the
-    // static TSI was not triggered by the given JCas cover class. So we return a default callsite
-    // and trust that it will be properly updated when the static TSI is committed.
-    if (staticTsi == null) {
+    // Use whichever staticTsi-equivalent instance has already been committed, if any. Reading the
+    // cached field directly (not via committedStaticTsi()) deliberately avoids forcing a commit
+    // from inside a built-in cover class's <clinit> -- that would reintroduce the issue-#234
+    // init-storm.
+    TypeSystemImpl committed = committedStaticTsi;
+    if (committed == null) {
+      // No commit has happened yet; return a default callsite that will be patched up by
+      // updateOrValidateAllCallSitesForJCasClass during the eventual commit.
       return createCallSite(clazz, featName);
     }
 
-    // If the given JCas cover class not registered yet, we also assume that the initialization of
-    // the static TSI was not triggered by the given JCas cover class.
     TypeImpl type;
     try {
       int typeId = clazz.getField("typeIndexID").getInt(null);
-      type = (typeId >= staticTsi.jcasRegisteredTypes.size()) ? null
-              : staticTsi.jcasRegisteredTypes.get(typeId);
+      type = (typeId >= committed.jcasRegisteredTypes.size()) ? null
+              : committed.jcasRegisteredTypes.get(typeId);
       if (type == null) {
         return createCallSite(clazz, featName);
       }

uimaj-core/src/test/java/org/apache/uima/cas/impl/FSClassRegistryTest.java

@@ -25,7 +25,7 @@
 import org.apache.uima.spi.SpiSentence;
 import org.apache.uima.spi.SpiToken;
 import org.apache.uima.util.CasCreationUtils;
-import org.apache.uima.util.Level;
+import org.assertj.core.api.AbstractBooleanAssert;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -45,7 +45,6 @@ void setup() {
 
   @Test
   void thatCreatingResourceManagersWithExtensionClassloaderDoesNotFillUpCache() throws Exception {
-    int numberOfCachedClassloadersAtStart = FSClassRegistry.clToType2JCasSize();
     for (int i = 0; i < 5; i++) {
       var resMgr = UIMAFramework.newDefaultResourceManager();
       resMgr.setExtensionClassLoader(getClass().getClassLoader(), true);
@@ -56,19 +55,20 @@ void thatCreatingResourceManagersWithExtensionClassloaderDoesNotFillUpCache() th
       assertThat(cl.getResource(FSClassRegistryTest.class.getName().replace(".", "/") + ".class")) //
               .isNotNull();
 
-      assertRegisteredClassLoaders(numberOfCachedClassloadersAtStart + 1,
-              "Only initial classloaders + the one owned by our ResourceManager");
+      assertClassLoaderRegistered(cl,
+              "Class loader owned by our ResourceManager should be registered after CAS creation")
+              .isTrue();
 
       resMgr.destroy();
 
-      assertRegisteredClassLoaders(numberOfCachedClassloadersAtStart, "Only initial classloaders");
+      assertClassLoaderRegistered(cl,
+              "Class loader owned by our ResourceManager should be unregistered after destroy")
+              .isFalse();
     }
   }
 
   @Test
   void thatCreatingResourceManagersWithExtensionPathDoesNotFillUpCache() throws Exception {
-    var numberOfCachedClassloadersAtStart = FSClassRegistry.clToType2JCasSize();
-
     for (int i = 0; i < 5; i++) {
       var resMgr = UIMAFramework.newDefaultResourceManager();
       resMgr.setExtensionClassPath("src/test/java", true);
@@ -78,12 +78,15 @@ void thatCreatingResourceManagersWithExtensionPathDoesNotFillUpCache() throws Ex
       assertThat(cl.getResource(FSClassRegistryTest.class.getName().replace(".", "/") + ".java")) //
               .isNotNull();
 
-      assertRegisteredClassLoaders(numberOfCachedClassloadersAtStart + 1,
-              "Only initial classloaders + the one owned by our ResourceManager");
+      assertClassLoaderRegistered(cl,
+              "Class loader owned by our ResourceManager should be registered after CAS creation")
+              .isTrue();
 
       resMgr.destroy();
 
-      assertRegisteredClassLoaders(numberOfCachedClassloadersAtStart, "Only initial classloaders");
+      assertClassLoaderRegistered(cl,
+              "Class loader owned by our ResourceManager should be unregistered after destroy")
+              .isFalse();
     }
   }
 
@@ -96,13 +99,17 @@ void thatJCasClassesCanBeLoadedThroughSPI() throws Exception {
             entry(SpiSentence.class.getName(), SpiSentence.class));
   }
 
-  private void assertRegisteredClassLoaders(int aExpectedCount, String aDescription) {
-    if (FSClassRegistry.clToType2JCasSize() > aExpectedCount) {
-      FSClassRegistry.log_registered_classloaders(Level.INFO);
-    }
-
-    assertThat(FSClassRegistry.clToType2JCasSize()) //
-            .as(aDescription) //
-            .isEqualTo(aExpectedCount);
+  // Note: we deliberately do not assert on the absolute number of registered class loaders
+  // (FSClassRegistry.clToType2JCasSize()). That cache is a process-wide static weak map shared with
+  // all other tests running in the same JVM. Other test classes leave class loaders registered
+  // there, and because the keys are weakly referenced they are reaped asynchronously by the GC -
+  // which makes the absolute size non-deterministic and was the cause of flaky failures. Instead we
+  // assert on the registration state of *our own* class loader, which we keep strongly reachable, so
+  // it is immune to other class loaders being reaped.
+
+  private AbstractBooleanAssert<?> assertClassLoaderRegistered(ClassLoader aClassLoader,
+          String aDescription) {
+    return assertThat(FSClassRegistry.isClToType2JCasRegistered(aClassLoader)) //
+            .as(aDescription);
   }
 }

uimaj-core/src/test/java/org/apache/uima/jcas/impl/LoadingBuiltinAnnotationBeforeCasTest.java

@@ -0,0 +1,104 @@
+/*
+ * 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 org.apache.uima.jcas.impl;
+
+import static java.lang.System.getProperty;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
+
+import java.io.ByteArrayOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.uima.jcas.tcas.Annotation;
+import org.apache.uima.resource.metadata.impl.TypeSystemDescription_impl;
+import org.apache.uima.util.CasCreationUtils;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Reproduces <a href="https://github.com/apache/uima-uimaj/issues/234">issue #234</a>: when a
+ * built-in Annotation cover class is touched before any CAS exists, the type system records a wrong
+ * (zero) jcasType for it and Annotation's feature callsites are never updated. Subsequent
+ * {@code getCasType} / {@code setBegin} calls then fail.
+ *
+ * <p>
+ * The bug only manifests in a fresh JVM where nothing has yet initialized {@link Annotation}. Class
+ * initialization is once-per-JVM, so running this in the shared Surefire JVM (where other tests
+ * have already created CASes and thereby initialized Annotation normally) would not reproduce it.
+ * The test therefore forks a dedicated JVM running {@link #main(String[])} and asserts on its exit
+ * code.
+ */
+public class LoadingBuiltinAnnotationBeforeCasTest {
+
+  /**
+   * Reproducer body. Must run in a fresh JVM (see the {@code @Test} below). Throws on failure so
+   * the JVM exits non-zero.
+   */
+  public static void main(String[] args) throws Exception {
+    // Trigger Annotation's <clinit> before any TypeSystemImpl/CAS exists -- the bug repro hinge.
+    Class.forName(Annotation.class.getName());
+
+    var tsd = new TypeSystemDescription_impl();
+    var jcas = CasCreationUtils.createCas(tsd, null, null).getJCas();
+
+    // (1) wrong jcasRegisteredTypes slot
+    jcas.getCasType(Annotation.type);
+
+    // (2) Annotation._FC_begin callsite stuck at -1
+    jcas.setDocumentText("hello");
+  }
+
+  @Test
+  void thatLoadingAnnotationBeforeCasDoesNotBreakTypeSystemManagement() throws Exception {
+    // Fork the same JVM that is running this test. ProcessHandle is best-effort, hence the
+    // fallback; the fallback path works on Windows too because CreateProcess auto-resolves .exe.
+    var java = ProcessHandle.current().info().command()
+            .orElse(getProperty("java.home") + "/bin/java");
+
+    var pb = new ProcessBuilder(java, "-cp", getProperty("java.class.path"), getClass().getName());
+    pb.redirectErrorStream(true);
+    var p = pb.start();
+
+    // Drain the subprocess output on a daemon thread so that a deadlocked subprocess that never
+    // emits EOF can't block this thread from reaching the timed waitFor below.
+    var buf = new ByteArrayOutputStream();
+    var drain = new Thread(() -> {
+      try {
+        p.getInputStream().transferTo(buf);
+      } catch (Exception ignored) {
+        // Subprocess died mid-transfer; whatever we captured is what we report.
+      }
+    }, "forked-jvm-output-drain");
+    drain.setDaemon(true);
+    drain.start();
+
+    if (!p.waitFor(60, TimeUnit.SECONDS)) {
+      p.destroyForcibly();
+      drain.join(5_000);
+      fail("forked JVM did not exit within 60s; partial output was:%n%s",
+              buf.toString(StandardCharsets.UTF_8));
+    }
+    drain.join(5_000);
+
+    assertThat(p.exitValue())
+            .as("forked JVM exited non-zero; subprocess output was:%n%s",
+                    buf.toString(StandardCharsets.UTF_8))
+            .isZero();
+  }
+}