This is an automated email from the ASF dual-hosted git repository.
kezhuw pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 427f959f22 ZOOKEEPER-5093: Ensure snapLog closed even if txnLog close
throws in FileTxnSnapLog.close
427f959f22 is described below
commit 427f959f22813673d6adc0c2265ac33cda4bc052
Author: Jeremy Schoemaker <[email protected]>
AuthorDate: Fri Sep 18 05:45:45 2026 -0500
ZOOKEEPER-5093: Ensure snapLog closed even if txnLog close throws in
FileTxnSnapLog.close
Reviewers: kezhuw
Author: shoemoney
Closes #2445 from shoemoney/fix/zk-close-leak
---
.../java/org/apache/zookeeper/common/IOUtils.java | 28 ++++++
.../server/persistence/FileTxnSnapLog.java | 10 +-
.../zookeeper/server/persistence/SnapShot.java | 4 +-
.../org/apache/zookeeper/common/IOUtilsTest.java | 110 +++++++++++++++++++++
.../server/persistence/FileTxnSnapLogTest.java | 31 ++++++
5 files changed, 175 insertions(+), 8 deletions(-)
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/common/IOUtils.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/common/IOUtils.java
index 7267d5420d..94de2eddb3 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/IOUtils.java
+++ b/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.
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java
index 2816826046..815c378321 100644
---
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java
+++
b/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")
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapShot.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapShot.java
index f5660c7df4..d2f3b3618e 100644
---
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapShot.java
+++
b/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;
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/common/IOUtilsTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/common/IOUtilsTest.java
new file mode 100644
index 0000000000..ac6ffd7de0
--- /dev/null
+++
b/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);
+ }
+
+}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
index 656eeb8a0a..ad4132748c 100644
---
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
+++
b/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();
