From 151aeed1abf5a533ed0b5e83fed7f71b3a684f7b Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Tue, 25 Aug 2026 15:54:56 -0500 Subject: [PATCH 1/3] fix(zookeeper): ensure snapLog closed even if txnLog close throws Fix verified RED->GREEN. FileTxnSnapLog.close leaks snapLog if txnLog close throws at FileTxnSnapLog.java:623 --- .../server/persistence/FileTxnSnapLog.java | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) 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 2816826046e..e84e8c0c8e2 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 @@ -621,16 +621,32 @@ public void rollLog() throws IOException { * @throws IOException */ public void close() throws IOException { + IOException firstEx = null; TxnLog txnLogToClose = txnLog; + txnLog = null; if (txnLogToClose != null) { - txnLogToClose.close(); + try { + txnLogToClose.close(); + } catch (IOException e) { + firstEx = e; + } } - txnLog = null; SnapShot snapSlogToClose = snapLog; + snapLog = null; if (snapSlogToClose != null) { - snapSlogToClose.close(); + try { + snapSlogToClose.close(); + } catch (IOException e) { + if (firstEx != null) { + firstEx.addSuppressed(e); + } else { + firstEx = e; + } + } + } + if (firstEx != null) { + throw firstEx; } - snapLog = null; } @SuppressWarnings("serial") From a1a8a3cbdf9daeddcf322a609188d82fcb47b753 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Tue, 25 Aug 2026 17:14:48 -0500 Subject: [PATCH 2/3] ci: retrigger Jenkins after flaky ZookeeperServerClusterTest From 505e71baaf02b1606b2f902452d5fdca43e43219 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Wed, 16 Sep 2026 17:47:48 -0500 Subject: [PATCH 3/3] fix: generalize persistence cleanup with IOUtils.closeAll --- .../org/apache/zookeeper/common/IOUtils.java | 28 +++++ .../server/persistence/FileTxnSnapLog.java | 26 +---- .../server/persistence/SnapShot.java | 4 +- .../apache/zookeeper/common/IOUtilsTest.java | 110 ++++++++++++++++++ .../persistence/FileTxnSnapLogTest.java | 31 +++++ 5 files changed, 175 insertions(+), 24 deletions(-) create mode 100644 zookeeper-server/src/test/java/org/apache/zookeeper/common/IOUtilsTest.java 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 7267d5420df..94de2eddb39 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 ignore 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 e84e8c0c8e2..815c3783214 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; @@ -621,32 +622,11 @@ public void rollLog() throws IOException { * @throws IOException */ public void close() throws IOException { - IOException firstEx = null; TxnLog txnLogToClose = txnLog; + SnapShot snapLogToClose = snapLog; txnLog = null; - if (txnLogToClose != null) { - try { - txnLogToClose.close(); - } catch (IOException e) { - firstEx = e; - } - } - SnapShot snapSlogToClose = snapLog; snapLog = null; - if (snapSlogToClose != null) { - try { - snapSlogToClose.close(); - } catch (IOException e) { - if (firstEx != null) { - firstEx.addSuppressed(e); - } else { - firstEx = e; - } - } - } - if (firstEx != null) { - throw firstEx; - } + 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 f5660c7df44..d2f3b3618e1 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 00000000000..ac6ffd7de01 --- /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 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 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 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 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 656eeb8a0aa..ad4132748cf 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();