You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@uima.apache.org by re...@apache.org on 2021/03/31 08:21:23 UTC
[uima-uimaj] 01/01: [UIMA-6348] Race-condition in TypeSystemImpl
commit
This is an automated email from the ASF dual-hosted git repository.
rec pushed a commit to branch bugfix/UIMA-6348-Race-condition-in-TypeSystemImpl-commit
in repository https://gitbox.apache.org/repos/asf/uima-uimaj.git
commit 8529ded28bd9ca2e1fc81cce382007fba289b57b
Author: Richard Eckart de Castilho <re...@apache.org>
AuthorDate: Wed Mar 31 10:21:10 2021 +0200
[UIMA-6348] Race-condition in TypeSystemImpl commit
- Perform the check if a type matches the expected type system not only in FsIndex_set_sorted.deleteFS but also in other deleteFS and insert methods
- Extract the check into a separate method
- Also perform the check when creating a feature structure via the CAS
- Added and documented option to disable strict checking
---
.../src/docs/asciidoc/uv3.migration.aids.adoc | 11 ++
.../java/org/apache/uima/cas/impl/CASImpl.java | 13 ++
.../java/org/apache/uima/cas/impl/FsIndex_bag.java | 2 +
.../apache/uima/cas/impl/FsIndex_set_sorted.java | 8 +-
.../apache/uima/cas/impl/FsIndex_singletype.java | 8 +-
.../org/apache/uima/cas/impl/TypeSystemImpl.java | 134 ++++++++++++---------
6 files changed, 109 insertions(+), 67 deletions(-)
diff --git a/uima-doc-v3-users-guide/src/docs/asciidoc/uv3.migration.aids.adoc b/uima-doc-v3-users-guide/src/docs/asciidoc/uv3.migration.aids.adoc
index 59f3f16..2746821 100644
--- a/uima-doc-v3-users-guide/src/docs/asciidoc/uv3.migration.aids.adoc
+++ b/uima-doc-v3-users-guide/src/docs/asciidoc/uv3.migration.aids.adoc
@@ -58,6 +58,17 @@ Existing type systems, if found, are reused.
Besides saving storage, this can sometimes improve locality of reference, and therefore, performance.
Setting this property disables this consolidation.
+Disable strict type source checking
+|
+
+`uima.disable_strict_type_source_check`
+
+Default: the type system a type belongs to must be exactly the type system instance of the CAS it is created in or added to.
+
+When creating a new feature structure or when adding or removing a feature structure to/from an index, it is checked that the
+type system the type belongs to is exactly the same instance as the type system of the CAS it is created in or the index it
+is added to. Due to the type system consolidation feature, this should always be the case. Setting this property disables the check.
+
|
Disable subtype of FSArray creation
diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/CASImpl.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/CASImpl.java
index eede0bf..b2b4f6f 100644
--- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/CASImpl.java
+++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/CASImpl.java
@@ -1409,6 +1409,8 @@ public class CASImpl extends AbstractCas_ImplBase implements CAS, CASMgr, LowLev
// must happen before the annotation is created, for compressed form 6 serialization order
// to insure sofa precedes the ref of it
}
+
+ assertTypeBelongsToCasTypesystem(ti);
FsGenerator3 g = svd.generators[ti.getCode()]; // get generator or null
@@ -1433,6 +1435,14 @@ public class CASImpl extends AbstractCas_ImplBase implements CAS, CASMgr, LowLev
// return (T) createFsFromGenerator(svd.generators, ti);
}
+ private void assertTypeBelongsToCasTypesystem(TypeImpl ti) {
+ if (!TypeSystemImpl.IS_DISABLE_STRICT_TYPE_SOURCE_CHECK && tsi_local != null && ti.getTypeSystem() != tsi_local) {
+ throw new IllegalArgumentException("Cannot create feature structure of type [" + ti.getName()
+ + "](" + ti.getCode() + ") from type system [" + ti.getTypeSystem()
+ + "] in CAS with typesystem [" + tsi_local + "]");
+ }
+ }
+
/**
* Called during construction of FS.
* For normal FS "new" operators, if in PEAR context, make the base version
@@ -1451,6 +1461,7 @@ public class CASImpl extends AbstractCas_ImplBase implements CAS, CASMgr, LowLev
TOP baseFs;
try {
suspendPearContext();
+ assertTypeBelongsToCasTypesystem(ti);
svd.reuseId = fs._id;
baseFs = createFsFromGenerator(svd.baseGenerators, ti);
} finally {
@@ -5633,6 +5644,8 @@ public BooleanArray emptyBooleanArray() {
case Slot_Short:
case Slot_Int: return Integer.toString(iv);
case Slot_Float: return Float.toString(int2float(iv));
+ default:
+ // Ignore
}
}
if (v instanceof Long) {
diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_bag.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_bag.java
index 2eed435..6da0e99 100644
--- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_bag.java
+++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_bag.java
@@ -85,6 +85,7 @@ public class FsIndex_bag<T extends FeatureStructure> extends FsIndex_singletype<
@Override
public final void insert(T fs) {
+ assertFsTypeMatchesIndexType(fs, "insert");
maybeCopy();
index.add((TOP) fs);
}
@@ -180,6 +181,7 @@ public class FsIndex_bag<T extends FeatureStructure> extends FsIndex_singletype<
*/
@Override
public boolean deleteFS(T fs) {
+ assertFsTypeMatchesIndexType(fs, "deleteFS");
maybeCopy();
return this.index.remove(fs);
}
diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_set_sorted.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_set_sorted.java
index 7f95bc6..4c0e671 100644
--- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_set_sorted.java
+++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_set_sorted.java
@@ -89,7 +89,7 @@ final public class FsIndex_set_sorted<T extends FeatureStructure> extends FsInde
*/
@Override
void insert(T fs) {
-
+ assertFsTypeMatchesIndexType(fs, "insert");
// past the initial load, or item is not > previous largest item to be added
maybeCopy();
if (isAnnotIdx) {
@@ -206,11 +206,7 @@ final public class FsIndex_set_sorted<T extends FeatureStructure> extends FsInde
*/
@Override
public boolean deleteFS(T fs) {
- if (((TOP)fs)._getTypeImpl() != this.type) {
- throw new IllegalArgumentException(
- String.format("Wrong type %s passed to deleteFS of index over type %s",
- ((TOP)fs)._getTypeImpl().getName(), this.type.getName()));
- }
+ assertFsTypeMatchesIndexType(fs, "deleteFS");
// maybeProcessBulkAdds(); // moved to OrderedFsSet_array class
maybeCopy();
return this.indexedFSs.remove(fs);
diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_singletype.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_singletype.java
index 95baf98..103817d 100644
--- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_singletype.java
+++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIndex_singletype.java
@@ -586,7 +586,13 @@ public abstract class FsIndex_singletype<T extends FeatureStructure>
@Override
public abstract int compare(FeatureStructure o1, FeatureStructure o2);
-
+ protected final void assertFsTypeMatchesIndexType(FeatureStructure fs, String operation) {
+ if (!TypeSystemImpl.IS_DISABLE_STRICT_TYPE_SOURCE_CHECK && ((TOP)fs)._getTypeImpl() != this.type) {
+ throw new IllegalArgumentException(
+ String.format("Wrong type %s passed to %s of index over type %s",
+ ((TOP)fs)._getTypeImpl().getName(), operation, this.type.getName()));
+ }
+ }
/// **
// * Common part of iterator creation
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 a136d5f..789331d 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
@@ -144,11 +144,23 @@ public class TypeSystemImpl implements TypeSystem, TypeSystemMgr, LowLevelTypeSy
public static final String DISABLE_TYPESYSTEM_CONSOLIDATION = "uima.disable_typesystem_consolidation";
/**
+ * Define this JVM property to disable strictly checking the source of a type with respect to its
+ * type system. Normally (i.e. when typesystem consolidation is enabled), types that are semantically
+ * equivalent should come from the same type system instances. There could be bugs (in client or framework
+ * code) or legacy code which simply ignores this requirement and still works. Therefore, this property
+ * allow to restore the old non-strict-checking behavior.
+ */
+ public static final String DISABLE_STRICT_TYPE_SOURCE_CHECK = "uima.disable_strict_type_source_check";
+
+ /**
* public for test case
*/
public static final boolean IS_DISABLE_TYPESYSTEM_CONSOLIDATION = // true || // debug
Misc.getNoValueSystemProperty(DISABLE_TYPESYSTEM_CONSOLIDATION);
-
+
+ public static final boolean IS_DISABLE_STRICT_TYPE_SOURCE_CHECK = // true || // debug
+ Misc.getNoValueSystemProperty(DISABLE_STRICT_TYPE_SOURCE_CHECK);
+
// private final static String DECOMPILE_JCAS = "uima.decompile.jcas";
// private final static boolean IS_DECOMPILE_JCAS = Misc.getNoValueSystemProperty(DECOMPILE_JCAS);
// private final static Set<String> decompiled = (IS_DECOMPILE_JCAS) ? new HashSet<String>(256) : null;
@@ -1383,73 +1395,73 @@ public class TypeSystemImpl implements TypeSystem, TypeSystemMgr, LowLevelTypeSy
// because it will call the type system iterator
// this.casMetadata.setupFeaturesAndCreatableTypes();
- if (!IS_DISABLE_TYPESYSTEM_CONSOLIDATION) {
- WeakReference<TypeSystemImpl> prevWr = committedTypeSystems.get(this);
- if (null != prevWr) {
- TypeSystemImpl prev = prevWr.get();
- if (null != prev) {
- // the following is a no-op if the generators already set up for this class loader
- prev.getGeneratorsForClassLoader(cl, false); // false - is not pear
-
- if (IS_TRACE_JCAS_EXPAND) {
- System.out.format("debug type system impl commit: consolidated a type system with a previous one, %d%n", prev.hashCode());
- System.out.println(Misc.getCallers(1, 50).toString());
- }
-
- return prev;
- }
- }
- }
-
- topType.computeDepthFirstCode(1); // fixes up ordering, supers before subs. also sets hasRef.
-
- computeFeatureOffsets(topType, 0);
-
-// if (IS_DECOMPILE_JCAS) {
-// synchronized(GLOBAL_TYPESYS_LOCK) {
-// for(Type t : getAllTypes()) {
-// decompile(t);
-// }
-// }
-// }
-
- type2jcci = FSClassRegistry.get_className_to_jcci(cl, false); // is not pear
- lookup = FSClassRegistry.getLookup(cl);
- cl_for_commit = cl;
- computeAdjustedFeatureOffsets(topType); // must preceed the FSClassRegistry JCas stuff below
-
- // Load all the available JCas classes (if not already loaded).
- // Has to follow above, because information computed above is used when
- // loading and checking JCas classes
-
-
- // not done here, done in caller (CASImpl commitTypeSystem),
- // when it gets the generators for the class loader for this type system for the first time.
-// fsClassRegistry = new FSClassRegistry(this, true);
-// FSClassRegistry.loadAtTypeSystemCommitTime(this, true, cl);
-
- this.locked = true;
-
- if (!IS_DISABLE_TYPESYSTEM_CONSOLIDATION) {
- committedTypeSystems.put(this, new WeakReference<>(this));
+ TypeSystemImpl maybeConsolidatedTypesystem;
+ if (IS_DISABLE_TYPESYSTEM_CONSOLIDATION) {
+ maybeConsolidatedTypesystem = finalizeCommit(cl);
+ } else {
+ maybeConsolidatedTypesystem = committedTypeSystems
+ .computeIfAbsent(this, _key -> new WeakReference<>(finalizeCommit(cl))).get();
}
- // this call is here for the case where a commit happens, but no subsequent
- // new CAS or switch classloader call is done. For example, a "reinit" in an existing CAS
+ // This call is here for the case where a commit happens, but no subsequent new CAS or switch
+ // classloader call is done. For example, a "reinit" in an existing CAS
// This call internally calls the code to load JCas classes for this class loader.
- getGeneratorsForClassLoader(cl, false); // false - is not pear
-// FSClassRegistry.loadJCasForTSandClassLoader(this, true, cl);
-
+ // FSClassRegistry.loadJCasForTSandClassLoader(this, true, cl);
+ // the following is a no-op if the generators already set up for this class loader
+ maybeConsolidatedTypesystem.getGeneratorsForClassLoader(cl, false); // false - is not pear
+
if (IS_TRACE_JCAS_EXPAND) {
- System.out.format("debug type system impl commited new type system and loaded JCas %d%n", this.hashCode());
- System.out.println(Misc.getCallers(1, 50).toString());
+ if (this == maybeConsolidatedTypesystem) {
+ System.out.format("debug type system impl commited new type system and loaded JCas %d%n",
+ this.hashCode());
+ System.out.println(Misc.getCallers(1, 50).toString());
+ } else {
+ System.out.format(
+ "debug type system impl commit: consolidated a type system with a previous one, %d%n",
+ maybeConsolidatedTypesystem.hashCode());
+ System.out.println(Misc.getCallers(1, 50).toString());
+ }
}
- return this;
+ return maybeConsolidatedTypesystem;
} // of sync block
}
+ private TypeSystemImpl finalizeCommit(ClassLoader cl) {
+ topType.computeDepthFirstCode(1); // fixes up ordering, supers before subs. also sets hasRef.
+
+ computeFeatureOffsets(topType, 0);
+
+// if (IS_DECOMPILE_JCAS) {
+// synchronized(GLOBAL_TYPESYS_LOCK) {
+// for(Type t : getAllTypes()) {
+// decompile(t);
+// }
+// }
+// }
+
+ type2jcci = FSClassRegistry.get_className_to_jcci(cl, false); // is not pear
+ lookup = FSClassRegistry.getLookup(cl);
+ cl_for_commit = cl;
+
+ computeAdjustedFeatureOffsets(topType); // must preceed the FSClassRegistry JCas stuff below
+
+ // Load all the available JCas classes (if not already loaded).
+ // Has to follow above, because information computed above is used when
+ // loading and checking JCas classes
+
+
+ // not done here, done in caller (CASImpl commitTypeSystem),
+ // when it gets the generators for the class loader for this type system for the first time.
+// fsClassRegistry = new FSClassRegistry(this, true);
+// FSClassRegistry.loadAtTypeSystemCommitTime(this, true, cl);
+
+ this.locked = true;
+
+ return this;
+ }
+
/**
* This is the actual offset for the feature, in either the int or ref array
*
@@ -2646,8 +2658,10 @@ public class TypeSystemImpl implements TypeSystem, TypeSystemMgr, LowLevelTypeSy
Class<? extends TOP> cls = JCasRegistry.getClassForIndex(typeindex);
if (cls != null) {
String className = cls.getName();
-// System.err.format("Missing UIMA type, JCas Class name: %s, index: %d, jcasRegisteredTypes size: %d%n", className, typeindex, jcasRegisteredTypes.size());
-// dumpTypeSystem();
+ System.err.format(
+ "Missing UIMA type, JCas Class name: %s, index: %d, jcasRegisteredTypes size: %d%n",
+ className, typeindex, jcasRegisteredTypes.size());
+ dumpTypeSystem();
throw new CASRuntimeException(CASRuntimeException.JCAS_TYPE_NOT_IN_CAS, className);
} else {
throw new CASRuntimeException(CASRuntimeException.JCAS_UNKNOWN_TYPE_NOT_IN_CAS);