ai-yang opened a new issue, #10747: URL: https://github.com/apache/rocketmq/issues/10747
## 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<ResponseFuture>` 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. -- 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]
