Close every FlowData, Pipeline and metadata collection, and fail the build when one is left open

ci/pmd-resource-ruleset.xml

@@ -0,0 +1,99 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ruleset name="51Degrees Resource Cleanup"
+         xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd">
+
+    <description>
+        Fails the build when a FlowData, a Pipeline or a collection of
+        metadata is not closed on every path. The native results in a FlowData
+        and the native metadata collections each hold the engine's data set,
+        so while one is open, closing the Pipeline cannot free the data set.
+        Java never frees the native results by itself.
+
+        Where something is closed by its owner elsewhere, such as the FlowData
+        the web integration closes at the end of a request, the code says so
+        with @SuppressWarnings and a comment naming the owner.
+    </description>
+
+    <rule name="FlowDataInTryWithResources"
+          language="java"
+          message="Create the FlowData in a try-with-resources statement, so its native results are freed on every path"
+          class="net.sourceforge.pmd.lang.rule.xpath.XPathRule">
+        <description>
+            Every FlowData created is closed by a try-with-resources statement,
+            unless it is returned to the caller or kept in a field.
+        </description>
+        <priority>3</priority>
+        <properties>
+            <property name="xpath">
+                <value>
+<![CDATA[
+(//MethodCall[@MethodName = 'createFlowData']
+ | //ConstructorCall[pmd-java:typeIs('fiftyone.pipeline.core.data.FlowData')])
+    [not(ancestor::Resource)]
+    [not(parent::ReturnStatement)]
+    [not(parent::AssignmentExpression)]
+]]>
+                </value>
+            </property>
+        </properties>
+    </rule>
+
+    <rule name="CloseableIterableInTryWithResources"
+          language="java"
+          message="Use the metadata collection in a try-with-resources statement, so it releases the data set on every path"
+          class="net.sourceforge.pmd.lang.rule.xpath.XPathRule">
+        <description>
+            Every CloseableIterable a method returns, such as the profiles or
+            the components of an engine, is closed by a try-with-resources
+            statement, unless it is returned to the caller.
+        </description>
+        <priority>3</priority>
+        <properties>
+            <property name="xpath">
+                <value>
+<![CDATA[
+//MethodCall[pmd-java:typeIs('fiftyone.pipeline.engines.fiftyone.data.CloseableIterable')]
+    [not(ancestor::Resource)]
+    [not(parent::ReturnStatement)]
+]]>
+                </value>
+            </property>
+        </properties>
+    </rule>
+
+    <rule name="PipelineClosed"
+          language="java"
+          message="Close the Pipeline in a try-with-resources statement or a finally block, so its elements and their data sets are freed on every path"
+          class="net.sourceforge.pmd.lang.rule.xpath.XPathRule">
+        <description>
+            A local variable holding a Pipeline that a builder has just built
+            is a try-with-resources resource, is closed in a finally block of
+            the same method, or is returned to the caller.
+        </description>
+        <priority>3</priority>
+        <properties>
+            <property name="xpath">
+                <value>
+<![CDATA[
+//LocalVariableDeclaration[not(parent::Resource)]
+    /VariableDeclarator[MethodCall
+        [@MethodName = ('build', 'buildFromConfiguration')]
+        [pmd-java:typeIs('fiftyone.pipeline.core.flowelements.Pipeline')]]
+    /VariableId[
+        let $name := @Name,
+            $body := ancestor::*[self::MethodDeclaration
+                or self::ConstructorDeclaration
+                or self::Initializer
+                or self::LambdaExpression][1]
+        return not($body//FinallyClause//MethodCall[@MethodName = 'close']
+                        /VariableAccess[@Name = $name])
+            and not($body//ReturnStatement/VariableAccess[@Name = $name])]
+]]>
+                </value>
+            </property>
+        </properties>
+    </rule>
+
+</ruleset>

device-detection/device-detection.hash.engine.on-premise/src/main/java/fiftyone/devicedetection/hash/engine/onpremise/data/ComponentMetaDataHash.java

@@ -22,6 +22,7 @@
 
 package fiftyone.devicedetection.hash.engine.onpremise.data;
 
+import fiftyone.pipeline.engines.fiftyone.data.CloseableIterable;
 import fiftyone.devicedetection.hash.engine.onpremise.flowelements.DeviceDetectionHashEngine;
 import fiftyone.devicedetection.hash.engine.onpremise.interop.PropertyIterable;
 import fiftyone.devicedetection.hash.engine.onpremise.interop.swig.ComponentMetaDataSwig;
@@ -77,7 +78,7 @@ public ProfileMetaData getDefaultProfile() {
     }
 
     @Override
-    public Iterable<FiftyOneAspectPropertyMetaData> getProperties() {
+    public CloseableIterable<FiftyOneAspectPropertyMetaData> getProperties() {
         return new PropertyIterable(
             engine,
             properties,

device-detection/device-detection.hash.engine.on-premise/src/main/java/fiftyone/devicedetection/hash/engine/onpremise/data/ProfileMetaDataHash.java

@@ -114,11 +114,19 @@ public ComponentMetaData getComponent() {
     @Override
     public String getName() {
         List<ValueMetaData> values = new ArrayList<>();
-        for (ValueMetaData value : getValues()) {
-            if (value.getProperty().getDisplayOrder() > 0 &&
-                value.getName().equalsIgnoreCase("N/A") == false) {
-                values.add(value);
+        // The native collection holds the data set until it is closed. The
+        // values taken from it are separate objects, so they outlive it.
+        try (CloseableIterable<ValueMetaData> profileValues = getValues()) {
+            for (ValueMetaData value : profileValues) {
+                if (value.getProperty().getDisplayOrder() > 0 &&
+                    value.getName().equalsIgnoreCase("N/A") == false) {
+                    values.add(value);
+                }
             }
+        } catch (RuntimeException e) {
+            throw e;
+        } catch (Exception e) {
+            throw new IllegalStateException(e);
         }
         ValueMetaData[] valuesArray = new ValueMetaData[0];
         valuesArray = values.toArray(valuesArray);

device-detection/device-detection.hash.engine.on-premise/src/main/java/fiftyone/devicedetection/hash/engine/onpremise/data/PropertyMetaDataHash.java

@@ -22,6 +22,7 @@
 
 package fiftyone.devicedetection.hash.engine.onpremise.data;
 
+import fiftyone.pipeline.engines.fiftyone.data.CloseableIterable;
 import fiftyone.devicedetection.hash.engine.onpremise.flowelements.DeviceDetectionHashEngine;
 import fiftyone.devicedetection.hash.engine.onpremise.interop.Swig;
 import fiftyone.devicedetection.hash.engine.onpremise.interop.ValueIterable;
@@ -136,7 +137,7 @@ public ComponentMetaData getComponent() {
     }
 
     @Override
-    public Iterable<ValueMetaData> getValues() {
+    public CloseableIterable<ValueMetaData> getValues() {
         return new ValueIterable(
             engine,
             engine.getMetaData().getValuesForProperty(source));

device-detection/device-detection.hash.engine.on-premise/src/test/java/fiftyone/devicedetection/hash/engine/onpremise/WrapperHash.java

@@ -28,6 +28,7 @@
 import fiftyone.pipeline.core.flowelements.Pipeline;
 import fiftyone.pipeline.core.flowelements.PipelineBuilder;
 import fiftyone.pipeline.engines.Constants;
+import fiftyone.pipeline.engines.fiftyone.data.CloseableIterable;
 import fiftyone.pipeline.engines.fiftyone.data.ComponentMetaData;
 import fiftyone.pipeline.engines.fiftyone.data.FiftyOneAspectPropertyMetaData;
 import fiftyone.pipeline.engines.fiftyone.data.ProfileMetaData;
@@ -70,15 +71,15 @@ public Iterable<FiftyOneAspectPropertyMetaData> getProperties() {
     }
 
     @Override
-    public Iterable<ValueMetaData> getValues() {
+    public CloseableIterable<ValueMetaData> getValues() {
         return engine.getValues();
     }
 
-    public Iterable<ProfileMetaData> getProfiles() {
+    public CloseableIterable<ProfileMetaData> getProfiles() {
         return engine.getProfiles();
     }
 
-    public Iterable<ComponentMetaData> getComponents() {
+    public CloseableIterable<ComponentMetaData> getComponents() {
         return engine.getComponents();
     }
 

device-detection/device-detection.hash.engine.on-premise/src/test/java/fiftyone/devicedetection/hash/engine/onpremise/data/DataValidatorHash.java

@@ -29,6 +29,7 @@
 import fiftyone.pipeline.core.data.FlowData;
 import fiftyone.pipeline.engines.data.AspectPropertyValue;
 import fiftyone.pipeline.engines.exceptions.NoValueException;
+import fiftyone.pipeline.engines.fiftyone.data.CloseableIterable;
 import fiftyone.pipeline.engines.fiftyone.data.ComponentMetaData;
 import fiftyone.pipeline.engines.fiftyone.data.FiftyOneAspectPropertyMetaData;
 
@@ -91,8 +92,16 @@ public void validateData(FlowData data, boolean validEvidence) throws NoValueExc
         // rather than a fixed value, and that valid evidence produces at least
         // one result.
         int componentCount = 0;
-        for (ComponentMetaData component : engine.getComponents()) {
-            componentCount++;
+        // The collection holds the engine's data set until it is closed.
+        try (CloseableIterable<ComponentMetaData> components =
+                 engine.getComponents()) {
+            for (ComponentMetaData component : components) {
+                componentCount++;
+            }
+        } catch (RuntimeException e) {
+            throw e;
+        } catch (Exception e) {
+            throw new IllegalStateException(e);
         }
         int matchedUserAgents = elementData.getUserAgents().getValue().size();
         assertTrue(

device-detection/device-detection.hash.engine.on-premise/src/test/java/fiftyone/devicedetection/hash/engine/onpremise/data/MetaDataHashTests.java

@@ -65,7 +65,7 @@ public void MetaData_Hash_Reload() throws ExecutionException, InterruptedExcepti
      * assumption rather than failing.
      */
     @Test
-    public void MetaData_Performance_Properties() {
+    public void MetaData_Performance_Properties() throws Exception {
         MetaDataHasherHash hasher = new MetaDataHasherHash();
         long start = System.currentTimeMillis();
         hasher.hashProperties(0, getWrapper());
@@ -83,7 +83,7 @@ public void MetaData_Performance_Properties() {
      * assumption rather than failing.
      */
     @Test
-    public void MetaData_Performance_Components() {
+    public void MetaData_Performance_Components() throws Exception {
         MetaDataHasherHash hasher = new MetaDataHasherHash();
         long start = System.currentTimeMillis();
         hasher.hashComponents(0, getWrapper());
@@ -101,7 +101,7 @@ public void MetaData_Performance_Components() {
      * assumption rather than failing.
      */
     @Test
-    public void MetaData_Performance_Values() {
+    public void MetaData_Performance_Values() throws Exception {
         MetaDataHasherHash hasher = new MetaDataHasherHash();
         long start = System.currentTimeMillis();
         hasher.hashValues(0, getWrapper());
@@ -119,7 +119,7 @@ public void MetaData_Performance_Values() {
      * assumption rather than failing.
      */
     @Test
-    public void MetaData_Performance_Profiles() {
+    public void MetaData_Performance_Profiles() throws Exception {
         MetaDataHasherHash hasher = new MetaDataHasherHash();
         long start = System.currentTimeMillis();
         hasher.hashProfiles(0, getWrapper());

device-detection/device-detection.hash.engine.on-premise/src/test/java/fiftyone/devicedetection/hash/engine/onpremise/data/MetaDataHasherHash.java

@@ -25,6 +25,7 @@
 import fiftyone.devicedetection.hash.engine.onpremise.flowelements.Constants;
 import fiftyone.devicedetection.shared.testhelpers.Wrapper;
 import fiftyone.devicedetection.shared.testhelpers.data.MetaDataHasher;
+import fiftyone.pipeline.engines.fiftyone.data.CloseableIterable;
 import fiftyone.pipeline.engines.fiftyone.data.ComponentMetaData;
 import fiftyone.pipeline.engines.fiftyone.data.FiftyOneAspectPropertyMetaData;
 import fiftyone.pipeline.engines.fiftyone.data.ProfileMetaData;
@@ -34,19 +35,21 @@
 public class MetaDataHasherHash implements MetaDataHasher {
 
     @Override
-    public int hashProperties(int hash, Wrapper wrapper) {
+    public int hashProperties(int hash, Wrapper wrapper) throws Exception {
         int i = 0;
         Iterable<FiftyOneAspectPropertyMetaData> properties = wrapper.getProperties();
         for (FiftyOneAspectPropertyMetaData property : properties) {
             if (i % 10 == 0) {
                 hash ^= property.hashCode();
 
                 int j = 0;
-                for (ValueMetaData value : property.getValues()) {
-                    if (j % 10 == 0) {
-                        hash ^= value.hashCode();
+                try (CloseableIterable<ValueMetaData> propertyValues = property.getValues()) {
+                    for (ValueMetaData value : propertyValues) {
+                        if (j % 10 == 0) {
+                            hash ^= value.hashCode();
+                        }
+                        j++;
                     }
-                    j++;
                 }
                 // Match metric property does not have component so don't include
                 if (!property.getCategory().equals(Constants.MatchMetrics.CATEGORY)) {
@@ -62,50 +65,60 @@ public int hashProperties(int hash, Wrapper wrapper) {
     }
 
     @Override
-    public int hashValues(int hash, Wrapper wrapper) {
+    public int hashValues(int hash, Wrapper wrapper) throws Exception {
         int i = 0;
-        for (ValueMetaData value : wrapper.getValues()) {
-            if (i % 100 == 0) {
-                hash ^= value.hashCode();
-                hash ^= value.getProperty().hashCode();
+        try (CloseableIterable<ValueMetaData> values = wrapper.getValues()) {
+            for (ValueMetaData value : values) {
+                if (i % 100 == 0) {
+                    hash ^= value.hashCode();
+                    hash ^= value.getProperty().hashCode();
+                }
+                i++;
             }
-            i++;
         }
         return hash;
     }
 
     @Override
-    public int hashComponents(int hash, Wrapper wrapper) {
-        for (ComponentMetaData component : wrapper.getComponents()) {
-            hash ^= component.hashCode();
-            int i = 0;
-            for (FiftyOneAspectPropertyMetaData property : component.getProperties()) {
-                if (i % 10 == 0) {
-                    hash ^= property.hashCode();
+    public int hashComponents(int hash, Wrapper wrapper) throws Exception {
+        try (CloseableIterable<ComponentMetaData> components = wrapper.getComponents()) {
+            for (ComponentMetaData component : components) {
+                hash ^= component.hashCode();
+                int i = 0;
+                try (CloseableIterable<FiftyOneAspectPropertyMetaData> componentProperties = component.getProperties()) {
+                    for (FiftyOneAspectPropertyMetaData property : componentProperties) {
+                        if (i % 10 == 0) {
+                            hash ^= property.hashCode();
+                        }
+                        i++;
+                    }
                 }
-                i++;
+                hash ^= component.getDefaultProfile().hashCode();
             }
-            hash ^= component.getDefaultProfile().hashCode();
         }
         return hash;
     }
 
     @Override
-    public int hashProfiles(int hash, Wrapper wrapper) {
+    public int hashProfiles(int hash, Wrapper wrapper) throws Exception {
         int i = 0;
-        for (ProfileMetaData profile : wrapper.getProfiles()) {
-            if (i % 100 == 0) {
-                hash ^= profile.hashCode();
-                hash ^= profile.getComponent().hashCode();
-                int j = 0;
-                for (ValueMetaData value : profile.getValues()) {
-                    if (j % 10 == 0) {
-                        hash ^= value.hashCode();
+        try (CloseableIterable<ProfileMetaData> profiles = wrapper.getProfiles()) {
+            for (ProfileMetaData profile : profiles) {
+                if (i % 100 == 0) {
+                    hash ^= profile.hashCode();
+                    hash ^= profile.getComponent().hashCode();
+                    int j = 0;
+                    try (CloseableIterable<ValueMetaData> profileValues = profile.getValues()) {
+                        for (ValueMetaData value : profileValues) {
+                            if (j % 10 == 0) {
+                                hash ^= value.hashCode();
+                            }
+                            j++;
+                        }
                     }
-                    j++;
                 }
+                i++;
             }
-            i++;
         }
         return hash;
     }

device-detection/device-detection.hash.engine.on-premise/src/test/java/fiftyone/devicedetection/hash/engine/onpremise/flowelements/EngineTests.java

@@ -62,9 +62,10 @@ private void testDataSourceTier(String fileName) throws Exception {
 			assumeTrue(e.getMessage(), false);
 		}
 
-		DeviceDetectionHashEngine engine = createEngine(dataFile);
-		String tier = engine.getDataSourceTier();
-		engine.close();
+		String tier;
+		try (DeviceDetectionHashEngine engine = createEngine(dataFile)) {
+			tier = engine.getDataSourceTier();
+		}
 
 		if (fileName.equals(LITE_HASH_DATA_FILE_PATH)) {
 			assertEquals("Lite", tier);