[Bug] invokeAsyncImpl violates InvokeCallback completion order
- 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
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