[jwrosewell]

What was wrong

The resource cleanup specification says that disposing a FlowData cleans up its element data, that element data and engines holding native memory must have a cleanup mechanism, and that the Pipeline cleans up the elements it contains. The required examples must show that every FlowData is disposed.

In Java the native results inside a FlowData are never freed by the garbage collector. The SWIG interface gives results, evidence, values and vectors no finalizer, so they are freed only when the FlowData is closed. Each native result holds the engine's data set and its resource manager, so one FlowData left open keeps the whole data set in memory after the Pipeline and the engine are closed, for as long as the process runs. The metadata collections an engine returns (profiles, components, values) hold the data set in the same way until they are closed, or until the garbage collector finalizes them.

A Java pipeline only closes its elements when it is built with setAutoCloseElements(true), which the pre-packaged builders and buildFromConfiguration do but new PipelineBuilder() does not.

What was found and fixed

In the library (7)

File (line on rc)	What was left open
pipeline.web/.../PipelineFilter.java (124 to 146)	The request's FlowData, whenever the rest of the filter chain, the Client Hints headers or the JavaScript serving threw. It is now closed in a finally block.
pipeline.web/.../services/PipelineResultServiceCore.java (75 to 84)	The FlowData, when adding evidence or processing threw, as it never reached the request. It is now closed before the exception is passed on.
pipeline.web.mvc/.../components/FiftyOneInterceptor.java (125 to 139)	The FlowData of every request for the JavaScript or JSON, and of any request where preHandle threw. Spring calls afterCompletion only when preHandle returns true. preHandle now closes it in every other case.
device-detection.hash.engine.on-premise/.../data/ProfileMetaDataHash.java (117)	The native values collection getName iterates.
ip-intelligence.engine.on-premise/.../data/ProfileMetaDataIPI.java (117)	The same.
pipeline.core/.../flowelements/PipelineBuilder.java, buildFromConfiguration (83 to 138)	The elements already built, whenever a later element failed to build or the pipeline could not be built from them, and the members of a parallel group whose later member failed. They are now closed before the failure is passed on. The original exception is the one thrown, and a failure to close an element is added to it as suppressed.
ComponentMetaData.getProperties(), ProfileMetaData.getValues(), ProfileMetaData.getValues(String), FiftyOneAspectPropertyMetaData.getValues()	Every collection they returned. They declared Iterable while the on-premise engines return native collections, two of which hold the data set, so a caller could only close one by casting. They now declare CloseableIterable, and every caller here closes it. See the release note below.

The declared types of those four methods are the only change to the public API.

In the examples (11)

File (line on rc)	What was left open
examples/device-detection/console/.../MetadataOnPrem.java (139, 148)	The profiles and the components collections, so closing the engine could not free its data set.
examples/device-detection/console/.../ShareUsageExample.java (139)	The ShareUsageElement and its sending thread, as the pipeline was not asked to close it. The comment said closing the pipeline flushes the queued data, which it did not.
examples/ip-intelligence/console/.../GettingStartedCloudMixed.java (147)	The three cloud engines.
examples/ip-intelligence/web/getting-started.api/.../GettingStartedApi.java (126 to 137)	The on-premise IP Intelligence engine and its data file, every time the servlet was destroyed, and everything built when init failed, as a servlet whose init fails is never destroyed.
location/geo-location.shell.examples/.../CombiningServices.java (151 to 176)	The three engines, and the pipeline on any exception.
location/geo-location.shell.examples/.../cloud/GettingStarted.java (95 to 119)	The pipeline on any exception.
pipeline.developer-examples.cloud-engine/.../Main.java (108), flowelement/.../Main.java (48), onpremise-engine/.../Main.java (50), usage-sharing/.../Main.java (92)	The pipeline and its elements, never closed.

Each example now builds its pipeline in a try-with-resources statement, and asks the pipeline to close the elements it was given. The metadata examples close the property and value collections as well.

The four IP Intelligence web examples (GettingStartedWebOnPrem, GettingStartedWebCloud, GettingStartedWebMixed, GettingStartedWebCloudMixed) closed the FlowData that the filter owns and closes at the end of the request, although their comments said not to. They no longer close it, and their comments now say that the filter does.

In the tests (106)

File (lines on rc)	What was left open
device-detection/.../DeviceDetectionOnPremisePipelineBuilderTest.java (23 tests, 90 to 321)	23 on-premise pipelines, each with the Hash data file loaded.
ip-intelligence/.../IPIntelligenceOnPremisePipelineBuilderTest.java (91 to 255, 162)	16 on-premise pipelines. The class is ignored at present, so these do not run.
pipeline.cloudrequestengine/.../CloudRequestEngineTests.java (78, 80, 116, 120, 333, 453, 482)	5 pipelines and 2 FlowData.
pipeline.cloudrequestengine/.../CloudAspectEngineBaseTests.java (373, 425, 483)	3 FlowData, and the pipeline each test built.
pipeline.core/.../FlowDataTests.java (323), ParallelElementsTests.java (129), PipelineTests.java (105)	3 FlowData.
pipeline.core/.../PipelineBuilderTests.java (419, 446, 484)	3 pipelines built from configuration. A new test builds a configuration whose second element fails, and checks that the first element was closed.
pipeline.core/.../integration/PipelineIntegrationTests.java (57, 74, 94, 116)	4 pipelines.
pipeline.jsonbuilder/.../JsonBuilderTests.java (463)	A pipeline, and the threads of the lazy loading engine in it.
pipeline.javascriptbuilder/.../CookieTests.java (141), JavaScriptUrlTests.java (74)	2 pipelines.
device-detection/.../CloudRequestOriginTests.java (89), ip-intelligence/.../CloudRequestOriginTests.java (91)	2 cloud pipelines.
pipeline.engines.fiftyone/.../SetHeadersElementTests.java (87, 107, 142, 170, 187, 210)	6 FlowData, and the pipeline each test built.
pipeline.engines.fiftyone/.../ShareUsageElementTests.java (16 tests, 253 to 1006)	16 FlowData, and in most tests the ShareUsageElement with its sending thread and the pipeline.
pipeline.engines.fiftyone/.../ShareUsageOverheadTests.java (114, 152)	11,000 FlowData closed only when nothing failed, and the pipeline with its ShareUsageElement. Closing that element sends what is queued, so it is now given an uploader that sends nothing anywhere.
pipeline.web/.../PipelineFilterTests.java (3 tests)	The pipeline each filter built, as no filter was destroyed.
device-detection.hash.engine.on-premise/.../EngineTests.java, ip-intelligence.engine.on-premise/.../EngineTests.java (65)	The engine, when reading its tier threw.
device-detection.hash.engine.on-premise/.../data/DataValidatorHash.java (94)	The components collection.
.../data/MetaDataHasherHash.java in both on-premise engines (67, 79, 96 and 62, 74, 91) and device-detection.shared/.../data/ValueTests.java (70)	7 values, profiles and components collections. The test wrappers now return them as CloseableIterable, so the callers can close them.

The guard

ci/pmd-resource-ruleset.xml adds three PMD rules, and every tree's pom runs them with maven-pmd-plugin once the tests are compiled, so the build fails on a new leak. The IP Intelligence tree runs them beside its existing unused code check.

1. FlowDataInTryWithResources fails when a FlowData is created by createFlowData() or new FlowDataDefault(...) outside a try-with-resources statement, unless it is returned or kept in a field.
2. CloseableIterableInTryWithResources fails when a call returning a CloseableIterable, such as an engine's profiles or components, is used outside a try-with-resources statement, unless it is returned.
3. PipelineClosed fails when a local Pipeline a builder has just built is neither a try-with-resources resource, nor closed in a finally block of the same method, nor returned.

The rules match how resources are created, so a mocked FlowData or Pipeline, or one borrowed from the web integration, is not reported. PMD's own CloseResource rule was tried first and left out. It reported every mock, and it treats a pipeline as closed whenever the pipeline is named in a try-with-resources statement, so try (FlowData data = pipeline.createFlowData()) hid a pipeline that was never closed. Where something is closed by its owner elsewhere, the code says so with @SuppressWarnings. That happens twice, where the result service hands its FlowData to the request, and where the overhead tests close their FlowData together so that closing is not timed.

The rules do not see a resource kept in a field, or a pipeline built by a helper method.

How it was checked

Nothing was built or run locally. Every check below ran on GitHub's runners through common-ci's pull request build (nightly-pull-request.yml, with the dry run on so that nothing is merged), on a throwaway branch carrying the named head with the macOS configurations taken out of ci/options.json, as the macOS runners were needed for package publishing at the time.

1. Finding the leaks. A build of every tree with PMD's CloseResource rule reporting every unclosed AutoCloseable (run 41818499245, 409 reports, most of them mocks or objects owned elsewhere), then builds reporting the three rules above without failing (41819956080, 41829473215, 41830100340 and, after the metadata types changed, 41870221674) until none was left.
2. The fix before rc moved. Head 69b631ab, built and tested with the rules failing the build, run 41871146691 passed on all 9 Linux and Windows configurations. Its first attempt failed on Ubuntu with Java 8 in the Selenium contract tests, where Firefox did not start, and the second attempt of the failed and cancelled jobs passed.
3. The fix as it will be released. Head 8cb65cf0, which merges the current rc (the data repository, AGENTS.md and package metadata changes) into this branch, run 41886371194 passed on all 9 Linux and Windows configurations at the first attempt.
4. The guard. A throwaway branch adding to pipeline.engines.fiftyone one test that builds a Pipeline and never closes it, creates 100 FlowData without closing them, and iterates an engine's profiles without closing the collection. On Ubuntu with Java 17, run 41851717793 failed the build in pipeline.engines.fiftyone with exactly three violations, PipelineClosed, FlowDataInTryWithResources and CloseableIterableInTryWithResources, one for each leak, after every module built before it had passed the check. The rules have not changed since.

The full build with the macOS configurations is run 41901498556.

Release note

• ComponentMetaData.getProperties(), ProfileMetaData.getValues(), ProfileMetaData.getValues(String) and FiftyOneAspectPropertyMetaData.getValues() now declare CloseableIterable rather than Iterable, which is what the on-premise engines have always returned. Close the collection, for example in a try-with-resources statement, so that it releases the engine's data set straight away. Source that assigns the result to an Iterable still compiles. Code compiled against the old declared types needs recompiling, and a class that implements one of these interfaces needs its method to declare CloseableIterable, so this is a small breaking change. The release stays a patch release.
• PipelineBuilder.buildFromConfiguration now closes the elements it has already built when building fails.
• The servlet filter and the Spring interceptor now close the request's FlowData on every path.

Not changed

• new PipelineBuilder() keeps its default of not closing the elements it is given unless asked.
• Tests holding only mocks, such as the element data built over mocked native results in DeviceDataTests and IPIntelligenceDataTests, hold nothing native and are unchanged.