apache / apache/rocketmq

[Bug] invokeAsyncImpl violates InvokeCallback completion order

Open
#10,747 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
22.6k
Forks
12k
Avg merge
3d 1h
Merged PRs (30d)
27

Description

## Affected branch

`develop` at `a06836dd564e5e43115493f775626cf98d51d10e`

## Problem

`InvokeCallback.operationComplete` documents that it must be invoked after either `operationSucceed` or `operationFail`. `ResponseFuture.executeInvokeCallback` follows that order.

However, `NettyRemotingAbstract.invokeAsyncImpl` currently builds this chain:

1. `whenComplete` invokes `operationComplete`.
2. `thenAccept` invokes `operationSucceed`.
3. `exceptionally` invokes `operationFail`.

This reverses the documented order on both success and failure paths.

It also mixes callback failures with invocation failures. If `operationComplete` throws after an otherwise successful invocation, the dependent stage becomes exceptional, `operationSucceed` is skipped, and the exception thrown by the user's completion callback is passed to `operationFail` as though the remoting operation had failed.

## Deterministic reproduction

The regression uses already-completed `CompletableFuture` instances, so it has no network, sleep, timer, or scheduling dependency.

For the invocation-failure path:

1. Complete the internal future exceptionally with a sentinel `RemotingException`.
2. Invoke `invokeAsyncImpl`.
3. Verify with Mockito `InOrder` that `operationFail` occurs before `operationComplete`.

On the unmodified branch this failed identically in 5/5 isolated JDK 8 Maven processes:

```text
Verification in order failure
Wanted operationComplete anywhere AFTER operationFail
```

A separate success-path regression makes `operationComplete` throw a sentinel exception. The current implementation deterministically produced exactly these interactions:

```text
operationComplete(responseFuture)
operationFail(operationComplete sentinel)
```

`operationSucceed` was never invoked.

## Impact

Callers cannot rely on the callback contract to publish the request outcome before final completion/cleanup. In addition, an exception from user callback code can:

- suppress a real success callback;
- trigger the opposite failure callback; and
- expose a callback implementation exception as a remoting failure.

## Expected behavior

- Invoke `operationSucceed` or `operationFail` according to the original invocation outcome.
- Invoke `operationComplete` afterward.
- Do not route exceptions thrown by callback methods into the opposite outcome callback.
- Preserve the raw-exception behavior introduced by #9119/#9120.

## Suggested fix

Use one terminal `whenComplete` callback:

- on success, invoke `operationSucceed` and then `operationComplete`;
- on invocation failure, build the existing synthetic `ResponseFuture`, invoke `operationFail(ExceptionUtils.getRealException(t))`, then invoke `operationComplete`;
- remove the downstream `thenAccept/exceptionally` chain that currently catches both invocation and callback failures.

This matches the existing order in `ResponseFuture.executeInvokeCallback`.

## Related work checked

- #7321/#7322 introduced the unified future callback API and its ordering documentation.
- #9119/#9120 only unwrap `CompletionException` before `operationFail`.
- Other `invokeAsyncImpl` issues concern semaphore handling, response-table cleanup, or retry behavior.

Immediate searches across open and closed issues and open, closed, and merged pull requests found no existing callback-order fix or assignee.

Contributor guide

Open the contributing guide

Research direction

Start in NettyRemotingAbstract.invokeAsyncImpl and compare its callback chain with ResponseFuture.executeInvokeCallback. Run the deterministic Mockito InOrder success and failure regressions described in the issue. Done means the original outcome callback runs before operationComplete, callback exceptions do not trigger the opposite outcome callback, and raw exceptions remain preserved.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.