(gravitino) branch main updated: [#10604] fix(core): Close all resources in BaseCatalog.close() even if one fails (#10609)

[yuqi4733]

This is an automated email from the ASF dual-hosted git repository.

yuqi4733 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git


The following commit(s) were added to refs/heads/main by this push:
     new ee26e09618 [#10604] fix(core): Close all resources in 
BaseCatalog.close() even if one fails (#10609)
ee26e09618 is described below

commit ee26e096187eb98d41169fc55043a59faaab83fb
Author: Sachin Ranjalkar <52783123+sachinn...@users.noreply.github.com>
AuthorDate: Wed Apr 1 19:43:32 2026 +0530

    [#10604] fix(core): Close all resources in BaseCatalog.close() even if one 
fails (#10609)
    
    ### What changes were proposed in this pull request?
    
    Wrap each resource close in `BaseCatalog.close()` in its own try/catch
    so that a failure in `ops.close()` does not prevent
    `authorizationPlugin` and `catalogCredentialManager` from being closed.
    The first caught exception is rethrown, with subsequent exceptions added
    as suppressed.
    
    ### Why are the changes needed?
    
    `BaseCatalog.close()` closes `ops`, `authorizationPlugin`, and
    `catalogCredentialManager` sequentially. If `ops.close()` throws, the
    remaining resources are never closed, causing resource leaks during
    catalog shutdown.
    
    Fix: #10604
    
    ### Does this PR introduce _any_ user-facing change?
    
    No. The only behavioral change is that all three resources are now
    always closed, even if an earlier close fails.
    
    ### How was this patch tested?
    
    Added a unit test (`testCloseClosesAllResourcesWhenOpsCloseFails`) in
    `TestBaseCatalog` that:
    1. Mocks `ops.close()` to throw `IOException`
    2. Verifies `authorizationPlugin.close()` and
    `credentialManager.close()` are still called
    3. Asserts the `IOException` is rethrown
    
    All tests in `TestBaseCatalog` pass. Formatted with `spotlessApply`.
---
 .../apache/gravitino/connector/BaseCatalog.java    | 30 +++++++++++++++++++---
 .../apache/gravitino/catalog/TestBaseCatalog.java  | 23 +++++++++++++++++
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java 
b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
index f681d36c66..2cfed0f645 100644
--- a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
+++ b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
@@ -287,18 +287,42 @@ public abstract class BaseCatalog<T extends BaseCatalog>
 
   @Override
   public void close() throws IOException {
+    IOException firstException = null;
     if (ops != null) {
-      ops.close();
+      try {
+        ops.close();
+      } catch (IOException e) {
+        firstException = e;
+      }
       ops = null;
     }
     if (authorizationPlugin != null) {
-      authorizationPlugin.close();
+      try {
+        authorizationPlugin.close();
+      } catch (IOException e) {
+        if (firstException != null) {
+          firstException.addSuppressed(e);
+        } else {
+          firstException = e;
+        }
+      }
       authorizationPlugin = null;
     }
     if (catalogCredentialManager != null) {
-      catalogCredentialManager.close();
+      try {
+        catalogCredentialManager.close();
+      } catch (Exception e) {
+        if (firstException != null) {
+          firstException.addSuppressed(e);
+        } else {
+          firstException = new IOException(e);
+        }
+      }
       catalogCredentialManager = null;
     }
+    if (firstException != null) {
+      throw firstException;
+    }
   }
 
   public Capability capability() {
diff --git 
a/core/src/test/java/org/apache/gravitino/catalog/TestBaseCatalog.java 
b/core/src/test/java/org/apache/gravitino/catalog/TestBaseCatalog.java
index 90a886db52..917d3b314a 100644
--- a/core/src/test/java/org/apache/gravitino/catalog/TestBaseCatalog.java
+++ b/core/src/test/java/org/apache/gravitino/catalog/TestBaseCatalog.java
@@ -20,10 +20,14 @@
 package org.apache.gravitino.catalog;
 
 import com.google.common.collect.ImmutableMap;
+import java.io.IOException;
+import org.apache.commons.lang3.reflect.FieldUtils;
 import org.apache.gravitino.TestCatalog;
 import org.apache.gravitino.connector.BaseCatalog;
 import org.apache.gravitino.connector.CatalogOperations;
 import org.apache.gravitino.connector.TestCatalogOperations;
+import org.apache.gravitino.connector.authorization.AuthorizationPlugin;
+import org.apache.gravitino.credential.CatalogCredentialManager;
 import org.apache.gravitino.meta.CatalogEntity;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -49,4 +53,23 @@ public class TestBaseCatalog {
     CatalogOperations dummyCatalogOperations = catalog2.ops();
     Assertions.assertTrue(dummyCatalogOperations instanceof 
DummyCatalogOperations);
   }
+
+  @Test
+  void testCloseClosesAllResourcesWhenOpsCloseFails() throws 
IllegalAccessException, IOException {
+    TestCatalog catalog = new TestCatalog();
+    CatalogOperations ops = Mockito.mock(CatalogOperations.class);
+    AuthorizationPlugin authorizationPlugin = 
Mockito.mock(AuthorizationPlugin.class);
+    CatalogCredentialManager credentialManager = 
Mockito.mock(CatalogCredentialManager.class);
+
+    Mockito.doThrow(new IOException("close ops failed")).when(ops).close();
+
+    FieldUtils.writeField(catalog, "ops", ops, true);
+    FieldUtils.writeField(catalog, "authorizationPlugin", authorizationPlugin, 
true);
+    FieldUtils.writeField(catalog, "catalogCredentialManager", 
credentialManager, true);
+
+    Assertions.assertThrows(IOException.class, catalog::close);
+
+    Mockito.verify(authorizationPlugin).close();
+    Mockito.verify(credentialManager).close();
+  }
 }