Resources not properly closed, resource & memory leaks

[snazy]

Describe the bug

Handling of closeable resources is not implemented in a couple of places in the Polaris code base. This leads to thread leaks and potentially other system resource leaks.

None of the resources held by "storage integrations" are ever closed.

[flyrain]

Thanks for reporting this! Can you point out the place where resources are not properly closed?

[flyrain]

Hi @snazy, given #1224 merged, can we close this? Let me know if there are other places still needed to be fixed.

[pingtimeout]

@flyrain Fwiw I did a check and could only find this

polaris/service/common/src/main/java/org/apache/polaris/service/context/catalog/PolarisCallContextCatalogFactory.java

Lines 91 to 100 in 24683d8

	IcebergCatalog catalogInstance =
	new IcebergCatalog(
	entityManager,
	metaStoreManagerFactory.getOrCreateMetaStoreManager(context.getRealmContext()),
	context,
	resolvedManifest,
	securityContext,
	taskExecutor,
	fileIOFactory,
	polarisEventListener);

place that looks suspicious. The IcebergCatalogAdapter implements Closeable but this method is never called. So even though all resources opened by the catalog adapter are added to a CloseableGroup, that group itself is never closed.

Could you double check and confirm whether this is correct?

[singhpk234]

[doubt] would the try-with-resources not call the this catalog instance close ?

polaris/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

Line 179 in aac2e85

try (IcebergCatalogHandler wrapper = newHandlerWrapper(securityContext, catalogName)) {

polaris/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

Line 1134 in aac2e85

public void close() throws Exception {

[adutra]

@pingtimeout I think @singhpk234 is right: each method in IcebergCatalogAdapter instantiates and closes an IcebergCatalogHandler using try-with-resources. Each IcebergCatalogHandler instance in turn may instantiate a Catalog, which gets closed when IcebergCatalogHandler is closed.

[pingtimeout]

Ah indeed, my bad. I overlooked the this.baseCatalog = assignment and the fact that the field is used in the close() method. All good then

[flyrain]

Closed it per community discussion