Skip to content

[fix][common] Handle synchronous failures from future suppliers#25939

Open
Radiancebobo wants to merge 5 commits into
apache:masterfrom
Radiancebobo:FuturesImprove
Open

[fix][common] Handle synchronous failures from future suppliers#25939
Radiancebobo wants to merge 5 commits into
apache:masterfrom
Radiancebobo:FuturesImprove

Conversation

@Radiancebobo

@Radiancebobo Radiancebobo commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Motivation

Some asynchronous helper methods accept a Supplier<CompletableFuture<T>> and assume the supplier always returns a future. However, a supplier can also fail synchronously before creating the future.

Before this change, those synchronous failures could bypass the expected asynchronous error handling path:

  • FutureUtil.Sequencer.sequential could throw directly instead of returning a failed future.
  • FutureUtil.composeAsync could throw inside the executor task and leave the returned future incomplete.
  • Futures.executeWithRetry in managed-ledger could throw before entering the retry state machine, so a retryable transient failure from the operation setup path would not be retried.

These helpers should preserve the CompletableFuture contract: callers should receive a completed or exceptionally completed future, rather than having synchronous exceptions escape or leave the returned future hanging.

Modifications

This change updates the future supplier handling paths to convert synchronous supplier failures into failed futures:

  • Added a private helper in FutureUtil to safely invoke Supplier<CompletableFuture<T>>.
  • Updated FutureUtil.Sequencer.sequential to use the safe supplier invocation path.
  • Updated FutureUtil.composeAsync to complete the returned future exceptionally when the supplier fails synchronously.
  • Updated managed-ledger Futures.executeWithRetry to route synchronous supplier failures through the existing retry logic.
  • Added null-future handling so suppliers that return null complete exceptionally with NullPointerException instead of causing an unchecked synchronous failure.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added FutureUtilTest.testSequencerReturnsFailedFutureWhenTaskThrowsSynchronously
  • Added FutureUtilTest.testComposeAsyncReturnsFailedFutureWhenSupplierThrowsSynchronously
  • Added FuturesTest.testExecuteWithRetryHandlesSynchronousFailure

Local verification:

  • ./gradlew :pulsar-common:test --tests org.apache.pulsar.common.util.FutureUtilTest -PtestRetryCount=0 --rerun-tasks
  • ./gradlew :managed-ledger:test --tests org.apache.bookkeeper.mledger.util.FuturesTest -PtestRetryCount=0 --rerun-tasks

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

@Radiancebobo

Copy link
Copy Markdown
Contributor Author

There's a similar PR #25931

Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java Outdated
Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java Outdated
Radiancebobo and others added 2 commits June 5, 2026 16:57
…reUtil.java

Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
…reUtil.java

Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
@Radiancebobo Radiancebobo requested a review from lhotari June 5, 2026 08:59
Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java Outdated
@Radiancebobo

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun

@Radiancebobo

Radiancebobo commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

I have executed these and made sure there are no issues.

./gradlew :pulsar-common:test :managed-ledger:test --rerun-tasks -PtestRetryCount=0 \
  --tests org.apache.pulsar.common.util.FutureUtilTest \
  --tests org.apache.bookkeeper.mledger.util.FuturesTest

./gradlew :pulsar-common:spotlessCheck :pulsar-common:checkstyleMain :pulsar-common:checkstyleTest

@Radiancebobo Radiancebobo requested a review from lhotari June 5, 2026 16:22
Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java Outdated
Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java Outdated
@Radiancebobo Radiancebobo requested a review from lhotari June 6, 2026 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants