From b545d2574176b5ef3ba19f49dd65b914a327f3a7 Mon Sep 17 00:00:00 2001 From: ohad Date: Mon, 7 Jul 2025 12:30:06 -0400 Subject: [PATCH 1/4] Implement and use a "closeAll" utility --- .../foundationdb/async/MoreAsyncUtil.java | 37 ++++++++++ .../foundationdb/async/MoreAsyncUtilTest.java | 72 +++++++++++++++++++ .../recordrepair/RecordRepair.java | 9 ++- .../throttled/ThrottledRetryingIterator.java | 21 +----- 4 files changed, 119 insertions(+), 20 deletions(-) diff --git a/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java b/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java index 563dec11a6..2744422fd4 100644 --- a/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java +++ b/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java @@ -912,6 +912,31 @@ public static void closeIterator(@Nonnull Iterator iterator) { } } + /** + * A utility to close multiple {@link AutoCloseable} objects, preserving all the caught exceptions. + * The method would attempt to close all closeables in order, even if some failed. + * @param closeables the given sequence of {@link AutoCloseable} + * @throws CloseException in case any exception was caught during the process. The first exception will be added + * as a {@code cause}. In case more than one exception was caught, it will be added as Suppressed. + */ + public static void closeAll(AutoCloseable... closeables) throws CloseException { + CloseException accumulatedException = null; + for (AutoCloseable closeable: closeables) { + try { + closeable.close(); + } catch (Exception e) { + if (accumulatedException == null) { + accumulatedException = new CloseException(e); + } else { + accumulatedException.addSuppressed(e); + } + } + } + if (accumulatedException != null) { + throw accumulatedException; + } + } + /** * This is supposed to replicate the semantics of {@link java.util.concurrent.CompletionStage#whenComplete(BiConsumer)} * but to handle the case where the completion handler might itself contain async work. @@ -1081,4 +1106,16 @@ private DeadlineExceededException(long deadlineTimeMillis) { addLogInfo("deadlineTimeMillis", deadlineTimeMillis); } } + + /** + * Exception thrown when the {@link #closeAll} method catches an exception. + * This exception will have the cause set to the first exception thrown during {@code closeAll} and any further + * exception thrown will be added as {@Suppressed}. + */ + @SuppressWarnings("serial") + public static class CloseException extends Exception { + public CloseException(final Throwable cause) { + super(cause); + } + } } diff --git a/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java b/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java index 24ca026af2..0903c684e8 100644 --- a/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java +++ b/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java @@ -24,6 +24,7 @@ import com.apple.test.ParameterizedTestUtils; import com.google.common.util.concurrent.ThreadFactoryBuilder; import org.hamcrest.Matcher; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -273,8 +274,56 @@ void swallowException() throws ExecutionException, InterruptedException { throw new RuntimeException(); }).get(); } + } + + @Test + void closeAllNoIssue() throws Exception { + SimpleCloseable c1 = new SimpleCloseable(false, null); + SimpleCloseable c2 = new SimpleCloseable(false, null); + SimpleCloseable c3 = new SimpleCloseable(false, null); + + MoreAsyncUtil.closeAll(c1, c2, c3); + + Assertions.assertTrue(c1.isClosed()); + Assertions.assertTrue(c2.isClosed()); + Assertions.assertTrue(c3.isClosed()); + } + + @Test + void closeAllFailed() throws Exception { + SimpleCloseable c1 = new SimpleCloseable(true, "c1"); + SimpleCloseable c2 = new SimpleCloseable(true, "c2"); + SimpleCloseable c3 = new SimpleCloseable(true, "c3"); + + final MoreAsyncUtil.CloseException exception = assertThrows(MoreAsyncUtil.CloseException.class, () -> MoreAsyncUtil.closeAll(c1, c2, c3)); + + Assertions.assertEquals("c1", exception.getCause().getMessage()); + final Throwable[] suppressed = exception.getSuppressed(); + Assertions.assertEquals(2, suppressed.length); + Assertions.assertEquals("c2", suppressed[0].getMessage()); + Assertions.assertEquals("c3", suppressed[1].getMessage()); + + Assertions.assertTrue(c1.isClosed()); + Assertions.assertTrue(c2.isClosed()); + Assertions.assertTrue(c3.isClosed()); + } + + @Test + void closeSomeFailed() throws Exception { + SimpleCloseable c1 = new SimpleCloseable(true, "c1"); + SimpleCloseable c2 = new SimpleCloseable(false, null); + SimpleCloseable c3 = new SimpleCloseable(true, "c3"); + + final MoreAsyncUtil.CloseException exception = assertThrows(MoreAsyncUtil.CloseException.class, () -> MoreAsyncUtil.closeAll(c1, c2, c3)); + Assertions.assertEquals("c1", exception.getCause().getMessage()); + final Throwable[] suppressed = exception.getSuppressed(); + Assertions.assertEquals(1, suppressed.length); + Assertions.assertEquals("c3", suppressed[0].getMessage()); + Assertions.assertTrue(c1.isClosed()); + Assertions.assertTrue(c2.isClosed()); + Assertions.assertTrue(c3.isClosed()); } private static void assertSwallowedOrNot(final CompletableFuture completedExceptionally1, final RuntimeException runtimeException1) throws InterruptedException, ExecutionException { @@ -295,4 +344,27 @@ private static Matcher isCurrentThreadNameOr(@Nonnull String threadName) private static Matcher isCurrentThreadNameOr(@Nonnull Matcher threadMatcher) { return either(threadMatcher).or(equalTo(Thread.currentThread().getName())); } + + private class SimpleCloseable implements AutoCloseable { + private final boolean fail; + private final String message; + private boolean closed = false; + + public SimpleCloseable(boolean fail, String message) { + this.fail = fail; + this.message = message; + } + + @Override + public void close() { + closed = true; + if (fail) { + throw new RuntimeException(message); + } + } + + public boolean isClosed() { + return closed; + } + } } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java index 639d8e8f8a..9281653890 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java @@ -115,7 +115,14 @@ public static Builder builder(@Nonnull FDBDatabase database, final FDBRecordStor @Override public void close() { - throttledIterator.close(); + try { + throttledIterator.close(); + } catch (Exception e) { + if (logger.isWarnEnabled()) { + logger.warn("Failed to close the throttled iterator", e); + } + // Do not rethrow. We are trying to close the runner and the exception should log all errors + } } @Nonnull diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java index 9333f30fe1..d6986be168 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java @@ -149,30 +149,13 @@ public CompletableFuture iterateAll(final FDBRecordStore.Builder storeBuil } @Override - public void close() { + public void close() throws MoreAsyncUtil.CloseException { if (closed) { return; } closed = true; // Ensure we call both close() methods, capturing all exceptions - RuntimeException caught = null; - try { - futureManager.close(); - } catch (RuntimeException e) { - caught = e; - } - try { - transactionalRunner.close(); - } catch (RuntimeException e) { - if (caught != null) { - caught.addSuppressed(e); - } else { - caught = e; - } - } - if (caught != null) { - throw caught; - } + MoreAsyncUtil.closeAll(futureManager, transactionalRunner); } /** From 64da875a891d474c1c86747f88c37c1c0a52919f Mon Sep 17 00:00:00 2001 From: ohad Date: Mon, 7 Jul 2025 15:20:33 -0400 Subject: [PATCH 2/4] Style --- .../main/java/com/apple/foundationdb/async/MoreAsyncUtil.java | 1 + 1 file changed, 1 insertion(+) diff --git a/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java b/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java index 2744422fd4..48885bb460 100644 --- a/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java +++ b/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java @@ -919,6 +919,7 @@ public static void closeIterator(@Nonnull Iterator iterator) { * @throws CloseException in case any exception was caught during the process. The first exception will be added * as a {@code cause}. In case more than one exception was caught, it will be added as Suppressed. */ + @SuppressWarnings("PMD.CloseResource") public static void closeAll(AutoCloseable... closeables) throws CloseException { CloseException accumulatedException = null; for (AutoCloseable closeable: closeables) { From a6c030570e5f4807b17e4bb404a14522f34a19ad Mon Sep 17 00:00:00 2001 From: ohad Date: Tue, 8 Jul 2025 14:55:36 -0400 Subject: [PATCH 3/4] PR comments: Move utility to CLoseableUtils. --- .../foundationdb/async/MoreAsyncUtil.java | 38 ------- .../foundationdb/util/CloseableUtils.java | 65 +++++++++++ .../foundationdb/async/MoreAsyncUtilTest.java | 72 ------------ .../foundationdb/util/CloseableUtilsTest.java | 105 ++++++++++++++++++ .../throttled/ThrottledRetryingIterator.java | 5 +- 5 files changed, 173 insertions(+), 112 deletions(-) create mode 100644 fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java create mode 100644 fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java diff --git a/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java b/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java index 48885bb460..563dec11a6 100644 --- a/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java +++ b/fdb-extensions/src/main/java/com/apple/foundationdb/async/MoreAsyncUtil.java @@ -912,32 +912,6 @@ public static void closeIterator(@Nonnull Iterator iterator) { } } - /** - * A utility to close multiple {@link AutoCloseable} objects, preserving all the caught exceptions. - * The method would attempt to close all closeables in order, even if some failed. - * @param closeables the given sequence of {@link AutoCloseable} - * @throws CloseException in case any exception was caught during the process. The first exception will be added - * as a {@code cause}. In case more than one exception was caught, it will be added as Suppressed. - */ - @SuppressWarnings("PMD.CloseResource") - public static void closeAll(AutoCloseable... closeables) throws CloseException { - CloseException accumulatedException = null; - for (AutoCloseable closeable: closeables) { - try { - closeable.close(); - } catch (Exception e) { - if (accumulatedException == null) { - accumulatedException = new CloseException(e); - } else { - accumulatedException.addSuppressed(e); - } - } - } - if (accumulatedException != null) { - throw accumulatedException; - } - } - /** * This is supposed to replicate the semantics of {@link java.util.concurrent.CompletionStage#whenComplete(BiConsumer)} * but to handle the case where the completion handler might itself contain async work. @@ -1107,16 +1081,4 @@ private DeadlineExceededException(long deadlineTimeMillis) { addLogInfo("deadlineTimeMillis", deadlineTimeMillis); } } - - /** - * Exception thrown when the {@link #closeAll} method catches an exception. - * This exception will have the cause set to the first exception thrown during {@code closeAll} and any further - * exception thrown will be added as {@Suppressed}. - */ - @SuppressWarnings("serial") - public static class CloseException extends Exception { - public CloseException(final Throwable cause) { - super(cause); - } - } } diff --git a/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java b/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java new file mode 100644 index 0000000000..f8f457e99b --- /dev/null +++ b/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java @@ -0,0 +1,65 @@ +/* + * CloseableUtils.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed 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 com.apple.foundationdb.util; + +public class CloseableUtils { + /** + * A utility to close multiple {@link AutoCloseable} objects, preserving all the caught exceptions. + * The method would attempt to close all closeables in order, even if some failed. + * @param closeables the given sequence of {@link AutoCloseable} + * @throws CloseException in case any exception was caught during the process. The first exception will be added + * as a {@code cause}. In case more than one exception was caught, it will be added as Suppressed. + */ + @SuppressWarnings("PMD.CloseResource") + public static void closeAll(AutoCloseable... closeables) throws CloseException { + CloseException accumulatedException = null; + for (AutoCloseable closeable: closeables) { + try { + closeable.close(); + } catch (Exception e) { + if (accumulatedException == null) { + accumulatedException = new CloseException(e); + } else { + accumulatedException.addSuppressed(e); + } + } + } + if (accumulatedException != null) { + throw accumulatedException; + } + } + + /** + * Exception thrown when the {@link CloseableUtils#closeAll} method catches an exception. + * This exception will have the {@code cause} set to the first exception thrown during {@code closeAll} and any further + * exception thrown will be added as {@code Suppressed}. + */ + @SuppressWarnings("serial") + public static class CloseException extends Exception { + public CloseException(final Throwable cause) { + super(cause); + } + } + + private CloseableUtils() { + // prevent constructor from being called + } +} diff --git a/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java b/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java index 0903c684e8..24ca026af2 100644 --- a/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java +++ b/fdb-extensions/src/test/java/com/apple/foundationdb/async/MoreAsyncUtilTest.java @@ -24,7 +24,6 @@ import com.apple.test.ParameterizedTestUtils; import com.google.common.util.concurrent.ThreadFactoryBuilder; import org.hamcrest.Matcher; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -274,56 +273,8 @@ void swallowException() throws ExecutionException, InterruptedException { throw new RuntimeException(); }).get(); } - } - - @Test - void closeAllNoIssue() throws Exception { - SimpleCloseable c1 = new SimpleCloseable(false, null); - SimpleCloseable c2 = new SimpleCloseable(false, null); - SimpleCloseable c3 = new SimpleCloseable(false, null); - - MoreAsyncUtil.closeAll(c1, c2, c3); - - Assertions.assertTrue(c1.isClosed()); - Assertions.assertTrue(c2.isClosed()); - Assertions.assertTrue(c3.isClosed()); - } - - @Test - void closeAllFailed() throws Exception { - SimpleCloseable c1 = new SimpleCloseable(true, "c1"); - SimpleCloseable c2 = new SimpleCloseable(true, "c2"); - SimpleCloseable c3 = new SimpleCloseable(true, "c3"); - - final MoreAsyncUtil.CloseException exception = assertThrows(MoreAsyncUtil.CloseException.class, () -> MoreAsyncUtil.closeAll(c1, c2, c3)); - - Assertions.assertEquals("c1", exception.getCause().getMessage()); - final Throwable[] suppressed = exception.getSuppressed(); - Assertions.assertEquals(2, suppressed.length); - Assertions.assertEquals("c2", suppressed[0].getMessage()); - Assertions.assertEquals("c3", suppressed[1].getMessage()); - - Assertions.assertTrue(c1.isClosed()); - Assertions.assertTrue(c2.isClosed()); - Assertions.assertTrue(c3.isClosed()); - } - - @Test - void closeSomeFailed() throws Exception { - SimpleCloseable c1 = new SimpleCloseable(true, "c1"); - SimpleCloseable c2 = new SimpleCloseable(false, null); - SimpleCloseable c3 = new SimpleCloseable(true, "c3"); - - final MoreAsyncUtil.CloseException exception = assertThrows(MoreAsyncUtil.CloseException.class, () -> MoreAsyncUtil.closeAll(c1, c2, c3)); - Assertions.assertEquals("c1", exception.getCause().getMessage()); - final Throwable[] suppressed = exception.getSuppressed(); - Assertions.assertEquals(1, suppressed.length); - Assertions.assertEquals("c3", suppressed[0].getMessage()); - Assertions.assertTrue(c1.isClosed()); - Assertions.assertTrue(c2.isClosed()); - Assertions.assertTrue(c3.isClosed()); } private static void assertSwallowedOrNot(final CompletableFuture completedExceptionally1, final RuntimeException runtimeException1) throws InterruptedException, ExecutionException { @@ -344,27 +295,4 @@ private static Matcher isCurrentThreadNameOr(@Nonnull String threadName) private static Matcher isCurrentThreadNameOr(@Nonnull Matcher threadMatcher) { return either(threadMatcher).or(equalTo(Thread.currentThread().getName())); } - - private class SimpleCloseable implements AutoCloseable { - private final boolean fail; - private final String message; - private boolean closed = false; - - public SimpleCloseable(boolean fail, String message) { - this.fail = fail; - this.message = message; - } - - @Override - public void close() { - closed = true; - if (fail) { - throw new RuntimeException(message); - } - } - - public boolean isClosed() { - return closed; - } - } } diff --git a/fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java b/fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java new file mode 100644 index 0000000000..5aa2915c3f --- /dev/null +++ b/fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java @@ -0,0 +1,105 @@ +/* + * CloseableUtilsTest.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed 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 com.apple.foundationdb.util; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Tests for the {@link CloseableUtils} class. + */ +public class CloseableUtilsTest { + + @Test + void closeAllNoIssue() throws Exception { + SimpleCloseable c1 = new SimpleCloseable(false, null); + SimpleCloseable c2 = new SimpleCloseable(false, null); + SimpleCloseable c3 = new SimpleCloseable(false, null); + + CloseableUtils.closeAll(c1, c2, c3); + + Assertions.assertTrue(c1.isClosed()); + Assertions.assertTrue(c2.isClosed()); + Assertions.assertTrue(c3.isClosed()); + } + + @Test + void closeAllFailed() throws Exception { + SimpleCloseable c1 = new SimpleCloseable(true, "c1"); + SimpleCloseable c2 = new SimpleCloseable(true, "c2"); + SimpleCloseable c3 = new SimpleCloseable(true, "c3"); + + final CloseableUtils.CloseException exception = assertThrows(CloseableUtils.CloseException.class, () -> CloseableUtils.closeAll(c1, c2, c3)); + + Assertions.assertEquals("c1", exception.getCause().getMessage()); + final Throwable[] suppressed = exception.getSuppressed(); + Assertions.assertEquals(2, suppressed.length); + Assertions.assertEquals("c2", suppressed[0].getMessage()); + Assertions.assertEquals("c3", suppressed[1].getMessage()); + + Assertions.assertTrue(c1.isClosed()); + Assertions.assertTrue(c2.isClosed()); + Assertions.assertTrue(c3.isClosed()); + } + + @Test + void closeSomeFailed() throws Exception { + SimpleCloseable c1 = new SimpleCloseable(true, "c1"); + SimpleCloseable c2 = new SimpleCloseable(false, null); + SimpleCloseable c3 = new SimpleCloseable(true, "c3"); + + final CloseableUtils.CloseException exception = assertThrows(CloseableUtils.CloseException.class, () -> CloseableUtils.closeAll(c1, c2, c3)); + + Assertions.assertEquals("c1", exception.getCause().getMessage()); + final Throwable[] suppressed = exception.getSuppressed(); + Assertions.assertEquals(1, suppressed.length); + Assertions.assertEquals("c3", suppressed[0].getMessage()); + + Assertions.assertTrue(c1.isClosed()); + Assertions.assertTrue(c2.isClosed()); + Assertions.assertTrue(c3.isClosed()); + } + + private class SimpleCloseable implements AutoCloseable { + private final boolean fail; + private final String message; + private boolean closed = false; + + public SimpleCloseable(boolean fail, String message) { + this.fail = fail; + this.message = message; + } + + @Override + public void close() { + closed = true; + if (fail) { + throw new RuntimeException(message); + } + } + + public boolean isClosed() { + return closed; + } + } +} diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java index d6986be168..9612ebc757 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java @@ -33,6 +33,7 @@ import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore; import com.apple.foundationdb.record.provider.foundationdb.runners.FutureAutoClose; import com.apple.foundationdb.record.provider.foundationdb.runners.TransactionalRunner; +import com.apple.foundationdb.util.CloseableUtils; import com.google.common.annotations.VisibleForTesting; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -149,13 +150,13 @@ public CompletableFuture iterateAll(final FDBRecordStore.Builder storeBuil } @Override - public void close() throws MoreAsyncUtil.CloseException { + public void close() throws CloseableUtils.CloseException { if (closed) { return; } closed = true; // Ensure we call both close() methods, capturing all exceptions - MoreAsyncUtil.closeAll(futureManager, transactionalRunner); + CloseableUtils.closeAll(futureManager, transactionalRunner); } /** From 9211256846980c89a06ac00ec581b38b903df765 Mon Sep 17 00:00:00 2001 From: ohad Date: Thu, 10 Jul 2025 17:47:59 -0400 Subject: [PATCH 4/4] PR comments --- .../foundationdb/util/CloseException.java | 33 +++++++++++++++++++ .../foundationdb/util/CloseableUtils.java | 28 +++++++++------- .../foundationdb/util/CloseableUtilsTest.java | 4 +-- .../recordrepair/RecordRepair.java | 12 ++----- .../throttled/ThrottledRetryingIterator.java | 3 +- 5 files changed, 56 insertions(+), 24 deletions(-) create mode 100644 fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseException.java diff --git a/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseException.java b/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseException.java new file mode 100644 index 0000000000..aa59565ea6 --- /dev/null +++ b/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseException.java @@ -0,0 +1,33 @@ +/* + * CloseException.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors + * + * Licensed 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 com.apple.foundationdb.util; + +/** + * Exception thrown when the {@link CloseableUtils#closeAll} method catches an exception. + * This exception will have the {@code cause} set to the first exception thrown during {@code closeAll} and any further + * exception thrown will be added as {@code Suppressed}. + */ +@SuppressWarnings("serial") +public class CloseException extends Exception { + public CloseException(final Throwable cause) { + super(cause); + } +} diff --git a/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java b/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java index f8f457e99b..4fb039408b 100644 --- a/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java +++ b/fdb-extensions/src/main/java/com/apple/foundationdb/util/CloseableUtils.java @@ -20,20 +20,36 @@ package com.apple.foundationdb.util; +import com.apple.foundationdb.annotation.API; + +/** + * Utility methods to help interact with {@link AutoCloseable} classes. + **/ public class CloseableUtils { /** * A utility to close multiple {@link AutoCloseable} objects, preserving all the caught exceptions. * The method would attempt to close all closeables in order, even if some failed. + * Note that {@link CloseException} is used to wrap any exception thrown during the closing process. The reason for + * that is the compiler fails to compile a {@link AutoCloseable#close()} implementation that throws a generic + * {@link Exception} (due to {@link InterruptedException} issue) - We therefore have to catch and wrap all exceptions. * @param closeables the given sequence of {@link AutoCloseable} * @throws CloseException in case any exception was caught during the process. The first exception will be added * as a {@code cause}. In case more than one exception was caught, it will be added as Suppressed. */ + @API(API.Status.INTERNAL) @SuppressWarnings("PMD.CloseResource") public static void closeAll(AutoCloseable... closeables) throws CloseException { CloseException accumulatedException = null; for (AutoCloseable closeable: closeables) { try { closeable.close(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + if (accumulatedException == null) { + accumulatedException = new CloseException(e); + } else { + accumulatedException.addSuppressed(e); + } } catch (Exception e) { if (accumulatedException == null) { accumulatedException = new CloseException(e); @@ -47,18 +63,6 @@ public static void closeAll(AutoCloseable... closeables) throws CloseException { } } - /** - * Exception thrown when the {@link CloseableUtils#closeAll} method catches an exception. - * This exception will have the {@code cause} set to the first exception thrown during {@code closeAll} and any further - * exception thrown will be added as {@code Suppressed}. - */ - @SuppressWarnings("serial") - public static class CloseException extends Exception { - public CloseException(final Throwable cause) { - super(cause); - } - } - private CloseableUtils() { // prevent constructor from being called } diff --git a/fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java b/fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java index 5aa2915c3f..0c87d16ffb 100644 --- a/fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java +++ b/fdb-extensions/src/test/java/com/apple/foundationdb/util/CloseableUtilsTest.java @@ -49,7 +49,7 @@ void closeAllFailed() throws Exception { SimpleCloseable c2 = new SimpleCloseable(true, "c2"); SimpleCloseable c3 = new SimpleCloseable(true, "c3"); - final CloseableUtils.CloseException exception = assertThrows(CloseableUtils.CloseException.class, () -> CloseableUtils.closeAll(c1, c2, c3)); + final CloseException exception = assertThrows(CloseException.class, () -> CloseableUtils.closeAll(c1, c2, c3)); Assertions.assertEquals("c1", exception.getCause().getMessage()); final Throwable[] suppressed = exception.getSuppressed(); @@ -68,7 +68,7 @@ void closeSomeFailed() throws Exception { SimpleCloseable c2 = new SimpleCloseable(false, null); SimpleCloseable c3 = new SimpleCloseable(true, "c3"); - final CloseableUtils.CloseException exception = assertThrows(CloseableUtils.CloseException.class, () -> CloseableUtils.closeAll(c1, c2, c3)); + final CloseException exception = assertThrows(CloseException.class, () -> CloseableUtils.closeAll(c1, c2, c3)); Assertions.assertEquals("c1", exception.getCause().getMessage()); final Throwable[] suppressed = exception.getSuppressed(); diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java index 9281653890..7ca244d8c2 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/recordrepair/RecordRepair.java @@ -30,6 +30,7 @@ import com.apple.foundationdb.record.provider.foundationdb.runners.throttled.CursorFactory; import com.apple.foundationdb.record.provider.foundationdb.runners.throttled.ThrottledRetryingIterator; import com.apple.foundationdb.tuple.Tuple; +import com.apple.foundationdb.util.CloseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -114,15 +115,8 @@ public static Builder builder(@Nonnull FDBDatabase database, final FDBRecordStor } @Override - public void close() { - try { - throttledIterator.close(); - } catch (Exception e) { - if (logger.isWarnEnabled()) { - logger.warn("Failed to close the throttled iterator", e); - } - // Do not rethrow. We are trying to close the runner and the exception should log all errors - } + public void close() throws CloseException { + throttledIterator.close(); } @Nonnull diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java index 9612ebc757..b47e2ba554 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/runners/throttled/ThrottledRetryingIterator.java @@ -33,6 +33,7 @@ import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore; import com.apple.foundationdb.record.provider.foundationdb.runners.FutureAutoClose; import com.apple.foundationdb.record.provider.foundationdb.runners.TransactionalRunner; +import com.apple.foundationdb.util.CloseException; import com.apple.foundationdb.util.CloseableUtils; import com.google.common.annotations.VisibleForTesting; import org.slf4j.Logger; @@ -150,7 +151,7 @@ public CompletableFuture iterateAll(final FDBRecordStore.Builder storeBuil } @Override - public void close() throws CloseableUtils.CloseException { + public void close() throws CloseException { if (closed) { return; }