ZOOKEEPER-5093: Ensure snapLog closed even if txnLog close throws in FileTxnSnapLog.close

zookeeper-server/src/main/java/org/apache/zookeeper/common/IOUtils.java

@@ -43,6 +43,34 @@ public static void closeStream(Closeable stream) {
         cleanup(null, stream);
     }
 
+    /**
+     * Closes every non-null object, preserving any {@link IOException} thrown.
+     *
+     * @param closeables
+     *            the objects to close, in order
+     * @throws IOException the first exception thrown while closing, with later
+     *            exceptions added as suppressed exceptions
+     */
+    public static void closeAll(Closeable... closeables) throws IOException {
+        IOException firstException = null;
+        for (Closeable closeable : closeables) {
+            if (closeable != null) {
+                try {
+                    closeable.close();
+                } catch (IOException e) {
+                    if (firstException == null) {
+                        firstException = e;
+                    } else if (firstException != e) {
+                        firstException.addSuppressed(e);
+                    }
+                }
+            }
+        }
+        if (firstException != null) {
+            throw firstException;
+        }
+    }
+
     /**
      * Close the Closeable objects and <b>ignore</b> any {@link IOException} or
      * null pointers. Must only be used for cleanup in exception handlers.

zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java

@@ -29,6 +29,7 @@
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.KeeperException.Code;
 import org.apache.zookeeper.ZooDefs.OpCode;
+import org.apache.zookeeper.common.IOUtils;
 import org.apache.zookeeper.common.Time;
 import org.apache.zookeeper.server.DataTree;
 import org.apache.zookeeper.server.DataTree.ProcessTxnResult;
@@ -622,15 +623,10 @@ public void rollLog() throws IOException {
      */
     public void close() throws IOException {
         TxnLog txnLogToClose = txnLog;
-        if (txnLogToClose != null) {
-            txnLogToClose.close();
-        }
+        SnapShot snapLogToClose = snapLog;
         txnLog = null;
-        SnapShot snapSlogToClose = snapLog;
-        if (snapSlogToClose != null) {
-            snapSlogToClose.close();
-        }
         snapLog = null;
+        IOUtils.closeAll(txnLogToClose, snapLogToClose);
     }
 
     @SuppressWarnings("serial")

zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapShot.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper.server.persistence;
 
+import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.util.Map;
@@ -28,7 +29,7 @@
  * implement this interface for implementing
  * snapshots.
  */
-public interface SnapShot {
+public interface SnapShot extends Closeable {
 
     /**
      * deserialize a data tree from the last valid snapshot and
@@ -67,6 +68,7 @@ public interface SnapShot {
      * free resources from this snapshot immediately
      * @throws IOException
      */
+    @Override
     void close() throws IOException;
 
 }

zookeeper-server/src/test/java/org/apache/zookeeper/common/IOUtilsTest.java

@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.zookeeper.ZKTestCase;
+import org.junit.jupiter.api.Test;
+
+public class IOUtilsTest extends ZKTestCase {
+
+    @Test
+    public void testCloseAllWithoutObjects() throws IOException {
+        IOUtils.closeAll();
+    }
+
+    @Test
+    public void testCloseAllInOrderIgnoringNulls() throws IOException {
+        List<Integer> closed = new ArrayList<>();
+
+        IOUtils.closeAll(null, () -> closed.add(1), null, () -> closed.add(2));
+
+        assertEquals(List.of(1, 2), closed);
+    }
+
+    @Test
+    public void testCloseAllPreservesFirstFailureAndSuppressesLaterFailures() {
+        IOException first = new IOException("first");
+        IOException second = new IOException("second");
+        IOException third = new IOException("third");
+        List<Integer> closed = new ArrayList<>();
+
+        IOException failure = assertThrows(IOException.class, () -> IOUtils.closeAll(
+            () -> {
+                closed.add(1);
+                throw first;
+            },
+            () -> {
+                closed.add(2);
+                throw second;
+            },
+            () -> {
+                closed.add(3);
+                throw third;
+            },
+            () -> closed.add(4)));
+
+        assertSame(first, failure);
+        assertArrayEquals(new Throwable[]{second, third}, failure.getSuppressed());
+        assertEquals(List.of(1, 2, 3, 4), closed);
+    }
+
+    @Test
+    public void testCloseAllPreservesFailureAfterSuccessfulClose() {
+        IOException expected = new IOException("second");
+        List<Integer> closed = new ArrayList<>();
+
+        IOException failure = assertThrows(IOException.class, () -> IOUtils.closeAll(
+            () -> closed.add(1),
+            () -> {
+                closed.add(2);
+                throw expected;
+            }));
+
+        assertSame(expected, failure);
+        assertEquals(0, failure.getSuppressed().length);
+        assertEquals(List.of(1, 2), closed);
+    }
+
+    @Test
+    public void testCloseAllDoesNotSuppressAnExceptionOnItself() {
+        IOException expected = new IOException("shared");
+        List<Integer> closed = new ArrayList<>();
+
+        IOException failure = assertThrows(IOException.class, () -> IOUtils.closeAll(
+            () -> {
+                throw expected;
+            },
+            () -> {
+                throw expected;
+            },
+            () -> closed.add(3)));
+
+        assertSame(expected, failure);
+        assertEquals(0, failure.getSuppressed().length);
+        assertEquals(List.of(3), closed);
+    }
+
+}

zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java

@@ -18,14 +18,20 @@
 
 package org.apache.zookeeper.server.persistence;
 
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
@@ -64,6 +70,31 @@ public class FileTxnSnapLogTest {
 
     private File snapVersionDir;
 
+    @Test
+    public void testCloseAttemptsBothLogsAndPreservesExceptions() throws IOException {
+        FileTxnSnapLog fileTxnSnapLog = new FileTxnSnapLog(logDir, snapDir);
+        fileTxnSnapLog.close();
+        TxnLog txnLog = mock(TxnLog.class);
+        SnapShot snapLog = mock(SnapShot.class);
+        IOException txnFailure = new IOException("transaction log close failed");
+        IOException snapFailure = new IOException("snapshot close failed");
+        doThrow(txnFailure).when(txnLog).close();
+        doThrow(snapFailure).when(snapLog).close();
+        fileTxnSnapLog.txnLog = txnLog;
+        fileTxnSnapLog.snapLog = snapLog;
+
+        IOException failure = assertThrows(IOException.class, fileTxnSnapLog::close);
+
+        verify(txnLog).close();
+        verify(snapLog).close();
+        assertSame(txnFailure, failure);
+        assertArrayEquals(new Throwable[]{snapFailure}, failure.getSuppressed());
+        assertNull(fileTxnSnapLog.txnLog);
+        assertNull(fileTxnSnapLog.snapLog);
+        fileTxnSnapLog.close();
+        verifyNoMoreInteractions(txnLog, snapLog);
+    }
+
     @BeforeEach
     public void setUp() throws Exception {
         tmpDir = ClientBase.createEmptyTestDir();