taklwu commented on code in PR #1341: URL: https://github.com/apache/ratis/pull/1341#discussion_r2898369270
########## ratis-common/src/main/java/org/apache/ratis/util/FutureUtils.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.ratis.util; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.function.BiConsumer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Helper class for processing futures. + */ +public final class FutureUtils { + + private static final Logger LOG = LoggerFactory.getLogger(FutureUtils.class); + + private FutureUtils() { + } + + /** + * This is method is used when you just want to add a listener to the given future. We will call + * {@link CompletableFuture#whenComplete(BiConsumer)} to register the {@code action} to the + * {@code future}. Ignoring the return value of a Future is considered as a bad practice as it may + * suppress exceptions thrown from the code that completes the future, and this method will catch + * all the exception thrown from the {@code action} to catch possible code bugs. + * <p/> + * And the error phone check will always report FutureReturnValueIgnored because every method in + * the {@link CompletableFuture} class will return a new {@link CompletableFuture}, so you always + * have one future that has not been checked. So we introduce this method and add a suppress + * warnings annotation here. + */ + @SuppressWarnings("FutureReturnValueIgnored") + public static <T> void addListener(CompletableFuture<T> future, + BiConsumer<? super T, ? super Throwable> action) { + future.whenComplete((resp, error) -> { + try { + // See this post on stack overflow(shorten since the url is too long), + // https://s.apache.org/completionexception + // For a chain of CompletableFuture, only the first child CompletableFuture can get the + // original exception, others will get a CompletionException, which wraps the original + // exception. So here we unwrap it before passing it to the callback action. + action.accept(resp, unwrapCompletionException(error)); + } catch (Throwable t) { + LOG.error("Unexpected error caught when processing CompletableFuture", t); + } + }); + } + + /** + * Get the cause of the {@link Throwable} if it is a {@link CompletionException}. + */ + public static Throwable unwrapCompletionException(Throwable error) { Review Comment: reusing existing method is good, sorry that I didn't know `unwrapCompletionException` was there. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
