diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java index 8ea3720d2..8da1d50b9 100644 --- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FSClassRegistry.java +++ b/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 type2jcci, ArrayList callSites_toSync) { String typeName = ti.getName(); diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemConstants.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemConstants.java index 0a0d34308..d4691e614 100644 --- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemConstants.java +++ b/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); } diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java index 644184461..408ddfa47 100644 --- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java +++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java @@ -2867,11 +2867,31 @@ public List 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 clazz, S */ public static final MutableCallSite createCallSiteForBuiltIn(Class 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 -- 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); } diff --git a/uimaj-core/src/test/java/org/apache/uima/cas/impl/FSClassRegistryTest.java b/uimaj-core/src/test/java/org/apache/uima/cas/impl/FSClassRegistryTest.java index 20490f020..2a8ba5537 100644 --- a/uimaj-core/src/test/java/org/apache/uima/cas/impl/FSClassRegistryTest.java +++ b/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); } } diff --git a/uimaj-core/src/test/java/org/apache/uima/jcas/impl/LoadingBuiltinAnnotationBeforeCasTest.java b/uimaj-core/src/test/java/org/apache/uima/jcas/impl/LoadingBuiltinAnnotationBeforeCasTest.java new file mode 100644 index 000000000..5374c58a0 --- /dev/null +++ b/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 issue #234: 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. + * + *

+ * 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 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(); + } +}