Improve handling of critical sections

Use LockableOntModel, etc., instead of Critical.

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/configuration/ConfigurationBeanLoader.java

@@ -2,19 +2,22 @@
 
 package edu.cornell.mannlib.vitro.webapp.utils.configuration;
 
+import static com.hp.hpl.jena.rdf.model.ResourceFactory.createResource;
+
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
-import static com.hp.hpl.jena.rdf.model.ResourceFactory.*;
+
 import javax.servlet.ServletContext;
 import javax.servlet.http.HttpServletRequest;
 
 import com.hp.hpl.jena.rdf.model.Model;
 import com.hp.hpl.jena.rdf.model.Resource;
-import com.hp.hpl.jena.rdf.model.ResourceFactory;
 import com.hp.hpl.jena.vocabulary.RDF;
 
-import edu.cornell.mannlib.vitro.webapp.utils.jena.Critical;
+import edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection.LockableModel;
+import edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection.LockedModel;
 
 /**
  * Load one or more Configuration beans from a specified model.
@@ -48,7 +51,7 @@ public class ConfigurationBeanLoader {
 	// ----------------------------------------------------------------------
 
 	/** Must not be null. */
-	private final Model model;
+	private final LockableModel locking;
 
 	/**
 	 * May be null, but the loader will be unable to satisfy instances of
@@ -63,26 +66,34 @@ public class ConfigurationBeanLoader {
 	private final HttpServletRequest req;
 
 	public ConfigurationBeanLoader(Model model) {
-		this(model, null, null);
+		this(new LockableModel(model), null, null);
+	}
+
+	public ConfigurationBeanLoader(LockableModel locking) {
+		this(locking, null, null);
 	}
 
 	public ConfigurationBeanLoader(Model model, ServletContext ctx) {
-		this(model, ctx, null);
+		this(new LockableModel(model), ctx, null);
+	}
+
+	public ConfigurationBeanLoader(LockableModel locking, ServletContext ctx) {
+		this(locking, ctx, null);
 	}
 
 	public ConfigurationBeanLoader(Model model, HttpServletRequest req) {
-		this(model,
+		this(new LockableModel(model), req);
-				(req == null) ? null : req.getSession().getServletContext(),
-				req);
 	}
 
-	private ConfigurationBeanLoader(Model model, ServletContext ctx,
+	public ConfigurationBeanLoader(LockableModel locking, HttpServletRequest req) {
+		this(locking, (req == null) ? null : req.getSession()
+				.getServletContext(), req);
+	}
+
+	private ConfigurationBeanLoader(LockableModel locking, ServletContext ctx,
 			HttpServletRequest req) {
-		if (model == null) {
+		this.locking = Objects.requireNonNull(locking,
-			throw new NullPointerException("model may not be null.");
+				"locking may not be null.");
-		}
-
-		this.model = model;
 		this.req = req;
 		this.ctx = ctx;
 	}
@@ -100,8 +111,8 @@ public class ConfigurationBeanLoader {
 		}
 
 		try {
-			ConfigurationRdf<T> parsedRdf = ConfigurationRdfParser.parse(model,
+			ConfigurationRdf<T> parsedRdf = ConfigurationRdfParser.parse(
-					uri, resultClass);
+					locking, uri, resultClass);
 			WrappedInstance<T> wrapper = InstanceWrapper.wrap(parsedRdf
 					.getConcreteClass());
 			wrapper.satisfyInterfaces(ctx, req);
@@ -120,9 +131,9 @@ public class ConfigurationBeanLoader {
 	public <T> Set<T> loadAll(Class<T> resultClass)
 			throws ConfigurationBeanLoaderException {
 		Set<String> uris = new HashSet<>();
-		try (Critical section = Critical.read(model)) {
+		try (LockedModel m = locking.read()) {
-			List<Resource> resources = model.listResourcesWithProperty(
+			List<Resource> resources = m.listResourcesWithProperty(RDF.type,
-					RDF.type, createResource(toJavaUri(resultClass))).toList();
+					createResource(toJavaUri(resultClass))).toList();
 			for (Resource r : resources) {
 				if (r.isURIResource()) {
 					uris.add(r.getURI());

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/configuration/ConfigurationRdfParser.java

@@ -11,9 +11,9 @@ import static edu.cornell.mannlib.vitro.webapp.utils.configuration.Configuration
 import java.lang.reflect.Modifier;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 
-import com.hp.hpl.jena.rdf.model.Model;
 import com.hp.hpl.jena.rdf.model.Property;
 import com.hp.hpl.jena.rdf.model.RDFNode;
 import com.hp.hpl.jena.rdf.model.Selector;
@@ -23,67 +23,63 @@ import com.hp.hpl.jena.vocabulary.RDF;
 
 import edu.cornell.mannlib.vitro.webapp.utils.configuration.PropertyType.PropertyStatement;
 import edu.cornell.mannlib.vitro.webapp.utils.configuration.PropertyType.PropertyTypeException;
-import edu.cornell.mannlib.vitro.webapp.utils.jena.Critical;
+import edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection.LockableModel;
+import edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection.LockedModel;
 
 /**
  * Parse the RDF for a single individual in the model to create a
  * ConfigurationRdf object.
  */
 public class ConfigurationRdfParser {
-	public static <T> ConfigurationRdf<T> parse(Model model, String uri,
+	public static <T> ConfigurationRdf<T> parse(LockableModel locking,
-			Class<T> resultClass) throws InvalidConfigurationRdfException {
+			String uri, Class<T> resultClass)
-		if (model == null) {
+			throws InvalidConfigurationRdfException {
-			throw new NullPointerException("model may not be null.");
+		Objects.requireNonNull(locking, "locking may not be null.");
-		}
+		Objects.requireNonNull(uri, "uri may not be null.");
-		if (uri == null) {
+		Objects.requireNonNull(resultClass, "resultClass may not be null.");
-			throw new NullPointerException("uri may not be null.");
-		}
-		if (resultClass == null) {
-			throw new NullPointerException("resultClass may not be null.");
-		}
 
-		confirmExistenceInModel(model, uri);
+		confirmExistenceInModel(locking, uri);
 
-		confirmEligibilityForResultClass(model, uri, resultClass);
+		confirmEligibilityForResultClass(locking, uri, resultClass);
 
-		Set<PropertyStatement> properties = loadProperties(model, uri);
+		Set<PropertyStatement> properties = loadProperties(locking, uri);
 
-		Class<? extends T> concreteClass = determineConcreteClass(model, uri,
+		Class<? extends T> concreteClass = determineConcreteClass(locking, uri,
 				resultClass);
 
 		return new ConfigurationRdf<T>(concreteClass, properties);
 	}
 
-	private static void confirmExistenceInModel(Model model, String uri)
+	private static void confirmExistenceInModel(LockableModel locking,
-			throws InvalidConfigurationRdfException {
+			String uri) throws InvalidConfigurationRdfException {
 		Selector s = new SimpleSelector(createResource(uri), null,
 				(RDFNode) null);
-		try (Critical section = Critical.read(model)) {
+		try (LockedModel m = locking.read()) {
-			if (model.listStatements(s).toList().isEmpty()) {
+			if (m.listStatements(s).toList().isEmpty()) {
 				throw individualDoesNotAppearInModel(uri);
 			}
 		}
 	}
 
-	private static void confirmEligibilityForResultClass(Model model,
+	private static void confirmEligibilityForResultClass(LockableModel locking,
 			String uri, Class<?> resultClass)
 			throws InvalidConfigurationRdfException {
 		Statement s = createStatement(createResource(uri), RDF.type,
 				createResource(toJavaUri(resultClass)));
-		try (Critical section = Critical.read(model)) {
+		try (LockedModel m = locking.read()) {
-			if (!model.contains(s)) {
+			if (!m.contains(s)) {
 				throw noTypeStatementForResultClass(s);
 			}
 		}
 	}
 
-	private static Set<PropertyStatement> loadProperties(Model model, String uri)
+	private static Set<PropertyStatement> loadProperties(LockableModel locking,
-			throws InvalidConfigurationRdfException {
+			String uri) throws InvalidConfigurationRdfException {
 		Set<PropertyStatement> set = new HashSet<>();
 
-		try (Critical section = Critical.read(model)) {
+		try (LockedModel m = locking.read()) {
-			List<Statement> rawStatements = model.listStatements(
+			List<Statement> rawStatements = m.listStatements(
-					model.getResource(uri), (Property) null, (RDFNode) null)
+					m.getResource(uri), (Property) null, (RDFNode) null)
 					.toList();
 			if (rawStatements.isEmpty()) {
 				throw noRdfStatements(uri);
@@ -106,14 +102,14 @@ public class ConfigurationRdfParser {
 		}
 	}
 
-	private static <T> Class<? extends T> determineConcreteClass(Model model,
+	private static <T> Class<? extends T> determineConcreteClass(
-			String uri, Class<T> resultClass)
+			LockableModel locking, String uri, Class<T> resultClass)
 			throws InvalidConfigurationRdfException {
 		Set<Class<? extends T>> concreteClasses = new HashSet<>();
 
-		try (Critical section = Critical.read(model)) {
+		try (LockedModel m = locking.read()) {
-			for (RDFNode node : model.listObjectsOfProperty(
+			for (RDFNode node : m.listObjectsOfProperty(createResource(uri),
-					createResource(uri), RDF.type).toSet()) {
+					RDF.type).toSet()) {
 				if (!node.isURIResource()) {
 					throw typeMustBeUriResource(node);
 				}

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/jena/Critical.java

@@ -1,39 +0,0 @@
-/* $This file is distributed under the terms of the license in /doc/license.txt$ */
-
-package edu.cornell.mannlib.vitro.webapp.utils.jena;
-
-import static com.hp.hpl.jena.shared.Lock.READ;
-import static com.hp.hpl.jena.shared.Lock.WRITE;
-
-import com.hp.hpl.jena.rdf.model.Model;
-
-/**
- * Use this in a try-with-resources block.
- * 
- * <pre>
- * try (Critical section = Critical.read(model)) {
- * }
- * </pre>
- */
-public class Critical implements AutoCloseable {
-	public static Critical read(Model model) {
-		return new Critical(model, READ);
-	}
-
-	public static Critical write(Model model) {
-		return new Critical(model, WRITE);
-	}
-	
-	private final Model model;
-
-	private Critical(Model model, boolean readLockRequested) {
-		this.model = model;
-		this.model.enterCriticalSection(readLockRequested);
-	}
-
-	@Override
-	public void close() {
-		this.model.leaveCriticalSection();
-	}
-
-}

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/jena/criticalsection/LockableModel.java

@@ -0,0 +1,39 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection;
+
+import java.util.Objects;
+
+import com.hp.hpl.jena.rdf.model.Model;
+import com.hp.hpl.jena.shared.Lock;
+
+/**
+ * Makes it easy to use a Jena Model in a try-with-resources block. At the end
+ * of the block, the close() method will not close the model, but will merely
+ * release the lock.
+ * 
+ * Wraps around a bare Model. Cannot be used without locking.
+ * 
+ * <pre>
+ * try (LockedModel m = new LockableModel(model).read()) {
+ *    ...
+ * }
+ * </pre>
+ */
+public class LockableModel {
+	private final Model model;
+
+	public LockableModel(Model model) {
+		this.model = Objects.requireNonNull(model, "model may not be null.");
+	}
+
+	public LockedModel read() {
+		model.enterCriticalSection(Lock.READ);
+		return new LockedModel(model);
+	}
+
+	public LockedModel write() {
+		model.enterCriticalSection(Lock.WRITE);
+		return new LockedModel(model);
+	}
+}

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/jena/criticalsection/LockableOntModel.java

@@ -0,0 +1,49 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection;
+
+import java.util.Objects;
+
+import com.hp.hpl.jena.ontology.OntModel;
+import com.hp.hpl.jena.shared.Lock;
+
+/**
+ * Makes it easy to use a Jena OntModel in a try-with-resources block. At the
+ * end of the block, the close() method will not close the model, but will
+ * merely release the lock.
+ * 
+ * Returned by the LockableOntModelSelector, or it can be wrapped around a bare
+ * OntModel. Cannot be used without locking.
+ * 
+ * <pre>
+ * try (LockedOntModel m = lockableOms.getDisplayModel.read()) {
+ *    ...
+ * }
+ * </pre>
+ * 
+ * or
+ * 
+ * <pre>
+ * try (LockedOntModel m = new LockableOntModel(ontModel).read()) {
+ *    ...
+ * }
+ * </pre>
+ */
+public class LockableOntModel {
+	private final OntModel ontModel;
+
+	public LockableOntModel(OntModel ontModel) {
+		this.ontModel = Objects.requireNonNull(ontModel,
+				"ontModel may not be null.");
+	}
+
+	public LockedOntModel read() {
+		ontModel.enterCriticalSection(Lock.READ);
+		return new LockedOntModel(ontModel);
+	}
+
+	public LockedOntModel write() {
+		ontModel.enterCriticalSection(Lock.WRITE);
+		return new LockedOntModel(ontModel);
+	}
+}

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/jena/criticalsection/LockableOntModelSelector.java

@@ -0,0 +1,53 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection;
+
+import java.util.Objects;
+
+import edu.cornell.mannlib.vitro.webapp.dao.jena.OntModelSelector;
+
+/**
+ * Makes it easy to use a Jena OntModel with a try-with-resources block. If you
+ * have access to an OntModelSelector, you can wrap it and then use it obtain
+ * LockableOntModels.
+ * 
+ * <pre>
+ * LockableOntModelSelector lockableOms = new LockingOntModelSelector(oms);
+ *  
+ * try (LockedOntModel m = lockableOms.getDisplayModel.read()) {
+ *    ...
+ * }
+ * </pre>
+ */
+public class LockableOntModelSelector {
+	private final OntModelSelector oms;
+
+	public LockableOntModelSelector(OntModelSelector oms) {
+		this.oms = Objects.requireNonNull(oms, "oms may not be null.");
+	}
+
+	public LockableOntModel getFullModel() {
+		return new LockableOntModel(oms.getFullModel());
+	}
+
+	public LockableOntModel getABoxModel() {
+		return new LockableOntModel(oms.getABoxModel());
+	}
+
+	public LockableOntModel getTBoxModel() {
+		return new LockableOntModel(oms.getTBoxModel());
+	}
+
+	public LockableOntModel getApplicationMetadataModel() {
+		return new LockableOntModel(oms.getApplicationMetadataModel());
+	}
+
+	public LockableOntModel getUserAccountsModel() {
+		return new LockableOntModel(oms.getUserAccountsModel());
+	}
+
+	public LockableOntModel getDisplayModel() {
+		return new LockableOntModel(oms.getDisplayModel());
+	}
+
+}

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/jena/criticalsection/LockedModel.java

@@ -0,0 +1,40 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection;
+
+import com.hp.hpl.jena.rdf.model.Model;
+
+import edu.cornell.mannlib.vitro.webapp.rdfservice.adapters.AbstractModelDecorator;
+
+/**
+ * A model that is easy to use in a try-with-resources code block. It can only
+ * be created by locking a LockableModel.
+ * 
+ * <pre>
+ * try (LockedModel m = lockableModel.read()) {
+ *    ...
+ * }
+ * </pre>
+ * 
+ * The close method has been hijacked to simply release the lock, and not to
+ * actually close the wrapped model.
+ */
+public class LockedModel extends AbstractModelDecorator implements
+		AutoCloseable {
+
+	/**
+	 * Should only be created by LockableModel.
+	 */
+	LockedModel(Model m) {
+		super(m);
+	}
+
+	/**
+	 * Just unlocks the model. Doesn't actually close it, because we may want to
+	 * use it again.
+	 */
+	@Override
+	public void close() {
+		super.leaveCriticalSection();
+	}
+}

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/jena/criticalsection/LockedOntModel.java

@@ -0,0 +1,32 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package edu.cornell.mannlib.vitro.webapp.utils.jena.criticalsection;
+
+import com.hp.hpl.jena.ontology.OntModel;
+
+import edu.cornell.mannlib.vitro.webapp.rdfservice.adapters.AbstractOntModelDecorator;
+
+/**
+ * A simple OntModel, except that it can only be created by locking a
+ * LockableOntModel. It is AutoCloseable, but the close method has been hijacked
+ * to simply release the lock, and not to actually close the wrapped model.
+ */
+public class LockedOntModel extends AbstractOntModelDecorator implements
+		AutoCloseable {
+
+	/**
+	 * Can only be created by LockableOntModel.
+	 */
+	LockedOntModel(OntModel m) {
+		super(m);
+	}
+
+	/**
+	 * Just unlocks the model. Doesn't actually close it, because we may want to
+	 * use it again.
+	 */
+	@Override
+	public void close() {
+		super.leaveCriticalSection();
+	}
+}

webapp/test/edu/cornell/mannlib/vitro/webapp/utils/configuration/ConfigurationBeanLoaderTest.java

@@ -101,7 +101,7 @@ public class ConfigurationBeanLoaderTest extends AbstractTestClass {
 		expectException(NullPointerException.class, "model may not be null");
 
 		@SuppressWarnings("unused")
-		Object unused = new ConfigurationBeanLoader(null);
+		Object unused = new ConfigurationBeanLoader((Model) null);
 	}
 
 	// ----------------------------------------------------------------------