-
Notifications
You must be signed in to change notification settings - Fork 8.9k
Feature 7840 saga annotation robustness #8027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.x
Are you sure you want to change the base?
Changes from all commits
24301a7
c1ef0b1
26eff1e
2513f05
0b64f2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,13 +16,15 @@ | |||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| package org.apache.seata.integration.tx.api.interceptor; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.ConfigurationKeys; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.Constants; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.exception.FrameworkException; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.exception.SkipCallbackWrapperException; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.executor.Callback; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.json.JsonUtil; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.util.CollectionUtils; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.common.util.NetUtil; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.config.ConfigurationFactory; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.core.context.RootContext; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.integration.tx.api.fence.DefaultCommonFenceHandler; | ||||||||||||||||||||||||||||||||||||
| import org.apache.seata.integration.tx.api.fence.hook.TccHook; | ||||||||||||||||||||||||||||||||||||
|
|
@@ -53,6 +55,9 @@ public class ActionInterceptorHandler { | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static final Logger LOGGER = LoggerFactory.getLogger(ActionInterceptorHandler.class); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private static final boolean ENABLE_ACTION_STATUS_REPORT = ConfigurationFactory.getInstance() | ||||||||||||||||||||||||||||||||||||
| .getBoolean(ConfigurationKeys.CLIENT_SAGA_ACTION_STATUS_REPORT_ENABLE, false); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Handler the Tx Aspect | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
|
|
@@ -111,7 +116,20 @@ public Object proceed( | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||
| // Execute business, and return the business result | ||||||||||||||||||||||||||||||||||||
| return targetCallback.execute(); | ||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||
| Object result = targetCallback.execute(); | ||||||||||||||||||||||||||||||||||||
| // Report action status: success (only for non-CommonFence mode) | ||||||||||||||||||||||||||||||||||||
| if (ENABLE_ACTION_STATUS_REPORT) { | ||||||||||||||||||||||||||||||||||||
| reportActionStatus(actionContext, Constants.ACTION_STATUS_SUCCESS); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||
| } catch (Throwable t) { | ||||||||||||||||||||||||||||||||||||
| // Report action status: failed (only for non-CommonFence mode) | ||||||||||||||||||||||||||||||||||||
| if (ENABLE_ACTION_STATUS_REPORT) { | ||||||||||||||||||||||||||||||||||||
| reportActionStatus(actionContext, Constants.ACTION_STATUS_FAILED); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| throw t; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -325,4 +343,32 @@ protected Map<String, Object> fetchActionRequestContext(Method method, Object[] | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return context; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Report action status to TC | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * @param actionContext the action context | ||||||||||||||||||||||||||||||||||||
| * @param status the action status (success/failed) | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| protected void reportActionStatus(BusinessActionContext actionContext, String status) { | ||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||
| actionContext.setActionStatus(status); | ||||||||||||||||||||||||||||||||||||
| actionContext.setUpdated(true); | ||||||||||||||||||||||||||||||||||||
| BusinessActionContextUtil.reportContext(actionContext); | ||||||||||||||||||||||||||||||||||||
| if (LOGGER.isDebugEnabled()) { | ||||||||||||||||||||||||||||||||||||
| LOGGER.debug( | ||||||||||||||||||||||||||||||||||||
| "Report action status: xid={}, branchId={}, status={}", | ||||||||||||||||||||||||||||||||||||
| actionContext.getXid(), | ||||||||||||||||||||||||||||||||||||
| actionContext.getBranchId(), | ||||||||||||||||||||||||||||||||||||
| status); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||||||||||||||||||||
| LOGGER.warn( | ||||||||||||||||||||||||||||||||||||
| "Report action status failed: xid={}, branchId={}, status={}, error={}", | ||||||||||||||||||||||||||||||||||||
| actionContext.getXid(), | ||||||||||||||||||||||||||||||||||||
| actionContext.getBranchId(), | ||||||||||||||||||||||||||||||||||||
| status, | ||||||||||||||||||||||||||||||||||||
| e.getMessage()); | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+366
to
+371
|
||||||||||||||||||||||||||||||||||||
| LOGGER.warn( | |
| "Report action status failed: xid={}, branchId={}, status={}, error={}", | |
| actionContext.getXid(), | |
| actionContext.getBranchId(), | |
| status, | |
| e.getMessage()); | |
| // Make status reporting best-effort: roll back the updated flag on failure | |
| actionContext.setUpdated(false); | |
| LOGGER.warn( | |
| "Report action status failed: xid={}, branchId={}, status={}", | |
| actionContext.getXid(), | |
| actionContext.getBranchId(), | |
| status, | |
| e); |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning log drops the exception stack trace, which makes diagnosing production/reporting failures harder. Pass e as the last argument to the logger (and optionally remove error={}/e.getMessage()), so stack traces are captured in logs.
| e.getMessage()); | |
| e.getMessage(), | |
| e); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| /* | ||
| * 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.seata.integration.tx.api.interceptor; | ||
|
|
||
| import org.apache.seata.common.Constants; | ||
| import org.apache.seata.common.executor.Callback; | ||
| import org.apache.seata.core.model.BranchType; | ||
| import org.apache.seata.integration.tx.api.fence.hook.TccHookManager; | ||
| import org.apache.seata.rm.tcc.api.BusinessActionContext; | ||
| import org.apache.seata.rm.tcc.api.BusinessActionContextUtil; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.MockedStatic; | ||
| import org.mockito.Mockito; | ||
|
|
||
| import java.lang.reflect.Field; | ||
| import java.lang.reflect.Method; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
|
|
||
| /** | ||
| * Tests for action status report functionality in ActionInterceptorHandler. | ||
| * | ||
| * Covers: | ||
| * 1. reportActionStatus method (success/failed/exception paths) | ||
| * 2. ENABLE_ACTION_STATUS_REPORT conditional branches in proceed method | ||
| */ | ||
| public class ActionInterceptorHandlerReportTest { | ||
|
|
||
| private MockedStatic<BusinessActionContextUtil> mockedContextUtil; | ||
| private boolean originalEnableActionStatusReport; | ||
|
|
||
| @BeforeEach | ||
| void setUp() throws Exception { | ||
| TccHookManager.clear(); | ||
| mockedContextUtil = Mockito.mockStatic(BusinessActionContextUtil.class); | ||
| mockedContextUtil | ||
| .when(() -> BusinessActionContextUtil.reportContext(any())) | ||
| .thenReturn(true); | ||
|
|
||
| // Enable action status report via reflection | ||
| originalEnableActionStatusReport = setEnableActionStatusReport(true); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void tearDown() throws Exception { | ||
| mockedContextUtil.close(); | ||
| TccHookManager.clear(); | ||
|
|
||
| // Restore original value | ||
| setEnableActionStatusReport(originalEnableActionStatusReport); | ||
| } | ||
|
|
||
| private boolean setEnableActionStatusReport(boolean value) throws Exception { | ||
| Field field = ActionInterceptorHandler.class.getDeclaredField("ENABLE_ACTION_STATUS_REPORT"); | ||
| field.setAccessible(true); | ||
|
|
||
| // Use sun.misc.Unsafe to modify static final field (works on all Java versions) | ||
| Field theUnsafe = sun.misc.Unsafe.class.getDeclaredField("theUnsafe"); | ||
| theUnsafe.setAccessible(true); | ||
| sun.misc.Unsafe unsafe = (sun.misc.Unsafe) theUnsafe.get(null); | ||
|
|
||
| Object base = unsafe.staticFieldBase(field); | ||
| long offset = unsafe.staticFieldOffset(field); | ||
| boolean original = unsafe.getBoolean(base, offset); | ||
| unsafe.putBoolean(base, offset, value); | ||
| return original; | ||
| } | ||
|
Comment on lines
+73
to
+87
|
||
|
|
||
| private BusinessActionContext createActionContext(String xid, long branchId) { | ||
| BusinessActionContext context = new BusinessActionContext(); | ||
| context.setXid(xid); | ||
| context.setBranchId(branchId); | ||
| context.setBranchType(BranchType.AT); | ||
| Map<String, Object> actionContext = new HashMap<>(); | ||
| context.setActionContext(actionContext); | ||
| return context; | ||
| } | ||
|
|
||
| private TwoPhaseBusinessActionParam createParam(String actionName) { | ||
| TwoPhaseBusinessActionParam param = new TwoPhaseBusinessActionParam(); | ||
| param.setActionName(actionName); | ||
| param.setDelayReport(Boolean.TRUE); | ||
| param.setBranchType(BranchType.AT); | ||
| param.setUseCommonFence(false); | ||
| return param; | ||
| } | ||
|
|
||
| // ---- Test reportActionStatus directly ---- | ||
|
|
||
| @Test | ||
| void testReportActionStatusSuccess() { | ||
| ActionInterceptorHandler handler = new ActionInterceptorHandler(); | ||
| BusinessActionContext context = createActionContext("xid1", 1L); | ||
|
|
||
| handler.reportActionStatus(context, Constants.ACTION_STATUS_SUCCESS); | ||
|
|
||
| assertEquals(Constants.ACTION_STATUS_SUCCESS, context.getActionStatus()); | ||
| mockedContextUtil.verify(() -> BusinessActionContextUtil.reportContext(context)); | ||
| } | ||
|
|
||
| @Test | ||
| void testReportActionStatusFailed() { | ||
| ActionInterceptorHandler handler = new ActionInterceptorHandler(); | ||
| BusinessActionContext context = createActionContext("xid2", 2L); | ||
|
|
||
| handler.reportActionStatus(context, Constants.ACTION_STATUS_FAILED); | ||
|
|
||
| assertEquals(Constants.ACTION_STATUS_FAILED, context.getActionStatus()); | ||
| } | ||
|
|
||
| @Test | ||
| void testReportActionStatusExceptionHandledGracefully() { | ||
| ActionInterceptorHandler handler = new ActionInterceptorHandler(); | ||
| BusinessActionContext context = createActionContext("xid3", 3L); | ||
|
|
||
| mockedContextUtil | ||
| .when(() -> BusinessActionContextUtil.reportContext(any())) | ||
| .thenThrow(new RuntimeException("report failed")); | ||
|
|
||
| assertDoesNotThrow(() -> handler.reportActionStatus(context, Constants.ACTION_STATUS_SUCCESS)); | ||
| assertEquals(Constants.ACTION_STATUS_SUCCESS, context.getActionStatus()); | ||
| } | ||
|
|
||
| // ---- Test ENABLE_ACTION_STATUS_REPORT conditional in proceed ---- | ||
|
|
||
| @Test | ||
| void testProceedReportsSuccessWhenCallbackSucceeds() throws Throwable { | ||
| ActionInterceptorHandler handler = Mockito.spy(new ActionInterceptorHandler()); | ||
| Method method = TestTarget.class.getDeclaredMethod("execute", BusinessActionContext.class); | ||
| BusinessActionContext context = createActionContext("xid100", 100L); | ||
| Object[] arguments = new Object[] {context}; | ||
| TwoPhaseBusinessActionParam param = createParam("testAction"); | ||
|
|
||
| Mockito.doReturn("branch123") | ||
| .when(handler) | ||
| .doTxActionLogStore( | ||
| any(Method.class), | ||
| any(), | ||
| any(TwoPhaseBusinessActionParam.class), | ||
| any(BusinessActionContext.class)); | ||
|
|
||
| Callback<Object> successCallback = () -> "ok"; | ||
| handler.proceed(method, arguments, "xid100", param, successCallback); | ||
|
|
||
| assertEquals(Constants.ACTION_STATUS_SUCCESS, context.getActionStatus()); | ||
| mockedContextUtil.verify(() -> BusinessActionContextUtil.reportContext(any()), Mockito.atLeast(1)); | ||
| } | ||
|
|
||
| @Test | ||
| void testProceedReportsFailedWhenCallbackThrows() throws Throwable { | ||
| ActionInterceptorHandler handler = Mockito.spy(new ActionInterceptorHandler()); | ||
| Method method = TestTarget.class.getDeclaredMethod("execute", BusinessActionContext.class); | ||
| BusinessActionContext context = createActionContext("xid200", 200L); | ||
| Object[] arguments = new Object[] {context}; | ||
| TwoPhaseBusinessActionParam param = createParam("testAction"); | ||
|
|
||
| Mockito.doReturn("branch456") | ||
| .when(handler) | ||
| .doTxActionLogStore( | ||
| any(Method.class), | ||
| any(), | ||
| any(TwoPhaseBusinessActionParam.class), | ||
| any(BusinessActionContext.class)); | ||
|
|
||
| Callback<Object> failCallback = () -> { | ||
| throw new RuntimeException("business error"); | ||
| }; | ||
|
|
||
| try { | ||
| handler.proceed(method, arguments, "xid200", param, failCallback); | ||
| } catch (RuntimeException e) { | ||
| // expected | ||
| } | ||
|
|
||
| assertEquals(Constants.ACTION_STATUS_FAILED, context.getActionStatus()); | ||
| } | ||
|
|
||
| // ---- Helper class for method reference ---- | ||
|
|
||
| public static class TestTarget { | ||
| public Object execute(BusinessActionContext context) { | ||
| return "result"; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENABLE_ACTION_STATUS_REPORTis read once at class-load time. This prevents config hot-reload from taking effect and can also capture the wrong value ifConfigurationFactoryisn’t initialized yet when this class is first loaded. Prefer reading the flag fromConfigurationFactory.getInstance()at execution time (or caching via a config listener) instead of astatic finalsnapshot.