bugfix: can't integrate dubbo with spring #6015

xxxcrel · 2023-11-10T10:41:21Z

I have registered the PR changes.

Ⅰ. Describe what this PR did

fix(#6014)

Ⅱ. Does this pull request fix one issue?

fixes #6014

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

CLAassistant · 2023-11-10T10:41:28Z

All committers have signed the CLA.

codecov · 2023-11-10T10:52:05Z

Codecov Report

Merging #6015 (770b77d) into 2.x (bbbbec7) will increase coverage by 0.00%.
The diff coverage is 26.08%.

Additional details and impacted files

@@            Coverage Diff            @@
##                2.x    #6015   +/-   ##
=========================================
  Coverage     49.57%   49.58%           
- Complexity     4747     4751    +4     
=========================================
  Files           907      907           
  Lines         31275    31284    +9     
  Branches       3770     3770           
=========================================
+ Hits          15506    15513    +7     
- Misses        14238    14244    +6     
+ Partials       1531     1527    -4

Files	Coverage Δ
...api/interceptor/parser/DefaultInterfaceParser.java	`68.75% <100.00%> (ø)`
...r/parser/GlobalTransactionalInterceptorParser.java	`35.71% <ø> (ø)`
...va/io/seata/integration/tx/api/util/ProxyUtil.java	`66.66% <100.00%> (+1.66%)`	⬆️
...interceptor/parser/TccActionInterceptorParser.java	`64.51% <100.00%> (ø)`
...tcc/resource/parser/TccRegisterResourceParser.java	`84.90% <100.00%> (ø)`
...erceptor/parser/DefaultResourceRegisterParser.java	`0.00% <0.00%> (ø)`
...va/io/seata/spring/tcc/TccAnnotationProcessor.java	`0.00% <0.00%> (ø)`
.../tx/api/remoting/parser/DefaultRemotingParser.java	`0.00% <0.00%> (ø)`
...ta/spring/annotation/GlobalTransactionScanner.java	`10.79% <0.00%> (-0.13%)`	⬇️
...ing/remoting/parser/RemotingFactoryBeanParser.java	`0.00% <0.00%> (ø)`

... and 6 files with indirect coverage changes

spring/src/main/java/io/seata/spring/annotation/GlobalTransactionScanner.java

spring/src/main/java/io/seata/spring/remoting/parser/RemotingFactoryBeanParser.java

tcc/src/main/java/io/seata/rm/tcc/interceptor/parser/TccActionInterceptorParser.java

integration-tx-api/src/main/java/io/seata/integration/tx/api/util/ProxyUtil.java

...api/src/main/java/io/seata/integration/tx/api/interceptor/parser/DefaultInterfaceParser.java

...ava/io/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java

spring/src/main/java/io/seata/spring/remoting/parser/RemotingFactoryBeanParser.java

xxxcrel · 2023-11-12T09:58:09Z

RemotingFactoryBeanParser的一些问题讨论
描述：目前2.x版本在集成spring的时候，因为spring和一些RPC框架机制的原因，有可能会需要拿到他的FactoryBean(因为原始的bean可能是生成的代理)再去交由其他的解析器进行真正的解析判断并获取相关RPC的信息，所以RemotingFactoryBeanParser需要先拿到FactoryBean再交给剩下的Parser去解析，但是目前的设计中RemotingFactoryBeanParser还会被重新调用一次解析，这会造成一个死循环调用。
基于跟社区的讨论，目前有两种方案：

在RemotingFactoryBeanParser中增加一个缓存，当重新调用的时候快速判断并跳出，然后继续剩下的Parser的逻辑
DefaultRemotingParser中增加isReference，isService, getServiceDesc的重构方法，后面都加上一个RemotingParser参数processedParser, 然后在后续Parser的遍历中跳过这个parser

xxxcrel · 2023-11-12T10:08:31Z

RemotingFactoryBeanParser的一些问题讨论描述：目前2.x版本在集成spring的时候，因为spring和一些RPC框架机制的原因，有可能会需要拿到他的FactoryBean(因为原始的bean可能是生成的代理)再去交由其他的解析器进行真正的解析判断并获取相关RPC的信息，所以RemotingFactoryBeanParser需要先拿到FactoryBean再交给剩下的Parser去解析，但是目前的设计中RemotingFactoryBeanParser还会被重新调用一次解析，这会造成一个死循环调用。基于跟社区的讨论，目前有两种方案：

在RemotingFactoryBeanParser中增加一个缓存，当重新调用的时候快速判断并跳出，然后继续剩下的Parser的逻辑

DefaultRemotingParser中增加isReference，isService, getServiceDesc的重构方法，后面都加上一个RemotingParser参数processedParser, 然后在后续Parser的遍历中跳过这个parser

使用第一种方案目前能规避spring集成的这个问题，但是之后可能其他的实现也会有类似的需求，那就需要开发者时刻注意到这个问题

funky-eyes

1.RemotingFactoryBeanParser 删除它的spi加载,改为在GlobalTransactionScanner#setApplicationContext中创建并赋值
2.DefaultRemotingParser增加addRemotingParser方法,将RemotingFactoryBeanParser放入allRemotingParsers
3.RemotingFactoryBeanParser#isReference中的 DefaultRemotingParser.get().isReference(bean, beanName); 改为 DefaultRemotingParser.get().isReference(factoryBean, factoryBeanName); 进行测试,是否会出现死循环,如果会则要进行接下来第四点的改造
4. RemotingFactoryBeanParser实例化的时候通过spi将其它4种实现加载,去除直接使用DefaultRemotingParser.get().isReference/isService方法,而是直接遍历4种实现的isReference/isService

funky-eyes · 2023-11-12T07:56:23Z

spring/src/main/java/io/seata/spring/remoting/parser/RemotingFactoryBeanParser.java

 import io.seata.integration.tx.api.remoting.RemotingDesc;
 import io.seata.integration.tx.api.remoting.parser.AbstractedRemotingParser;
 import io.seata.integration.tx.api.remoting.parser.DefaultRemotingParser;
 import io.seata.spring.util.SpringProxyUtils;
 import org.springframework.context.ApplicationContext;

+import java.util.Collections;


Introducing java-related packages should be placed at the top

xxxcrel · 2023-11-13T14:19:16Z

1.RemotingFactoryBeanParser 删除它的spi加载,改为在GlobalTransactionScanner#setApplicationContext中创建并赋值 2.DefaultRemotingParser增加addRemotingParser方法,将RemotingFactoryBeanParser放入allRemotingParsers 3.RemotingFactoryBeanParser#isReference中的 DefaultRemotingParser.get().isReference(bean, beanName); 改为 DefaultRemotingParser.get().isReference(factoryBean, factoryBeanName); 进行测试,是否会出现死循环,如果会则要进行接下来第四点的改造 4. RemotingFactoryBeanParser实例化的时候通过spi将其它4种实现加载,去除直接使用DefaultRemotingParser.get().isReference/isService方法,而是直接遍历4种实现的isReference/isService

经过测试，使用beanFactoryName可以跳出循环

funky-eyes

LGTM

wt-better · 2023-11-14T02:30:11Z

...ion-tx-api/src/main/java/io/seata/integration/tx/api/interceptor/parser/InterfaceParser.java

@@ -22,7 +22,7 @@
 */
 public interface InterfaceParser {

-    ProxyInvocationHandler parserInterfaceToProxy(Object target) throws Exception;
+    ProxyInvocationHandler parserInterfaceToProxy(Object target, String beanName) throws Exception;


2.x 目前和spring解耦的，beanName是spring的定义。我们这里设置成objectName是不是更好，在spring的场景下：objectName = beanName

wt-better · 2023-11-14T02:41:17Z

...-tx-api/src/main/java/io/seata/integration/tx/api/remoting/parser/DefaultRemotingParser.java

+     * register custom remoting parser
+     * @param remotingParser
+     */
+    public void registerRemotingParser(RemotingParser remotingParser) {


注册方法最好加锁（因为可以牵扯到并发注册），或者使用copyOnwrite的方式

wt-better · 2023-11-14T02:44:14Z

spring/src/main/java/io/seata/spring/remoting/parser/RemotingFactoryBeanParser.java

@@ -36,7 +40,7 @@ public class RemotingFactoryBeanParser extends AbstractedRemotingParser {
     * @param beanName           the bean name
     * @return boolean boolean
     */
-    protected static Object getRemotingFactoryBean(Object bean, String beanName) {
+    protected Object getRemotingFactoryBean(Object bean, String beanName) {


我们是不是在构造方法里面加一个applicationContext!=null的check（防止一些spring异常场景，applicationContext没set进去），那么getRemotingFactoryBean里面的applicationContext!=null是可以去除掉的。

另外factoryBeanName的构建落在多个方法里面，是否能够抽出一个统一的方法

leezongjie

GLTM

wt-better · 2023-11-14T06:31:51Z

LGTM

fix can't parse FactoryBean with spring

69c1714

funky-eyes reviewed Nov 11, 2023

View reviewed changes

spring/src/main/java/io/seata/spring/annotation/GlobalTransactionScanner.java Outdated Show resolved Hide resolved

spring/src/main/java/io/seata/spring/remoting/parser/RemotingFactoryBeanParser.java Outdated Show resolved Hide resolved

funky-eyes added this to the 2.0.0 milestone Nov 11, 2023

funky-eyes added type: bug Category issues or prs related to bug. module/spring spring module labels Nov 11, 2023

funky-eyes and others added 6 commits November 11, 2023 17:33

Merge branch '2.x' into fix-spring-integration

fd0a404

use ObjectHolder

3e2bd44

fix can't get actually beanName when bean is proxied

f3793c2

fix recursive call

39391d0

fix wrong name

b3f1002

fix checkstyle

1eca9c2

xxxcrel requested a review from funky-eyes November 11, 2023 14:53

funky-eyes reviewed Nov 11, 2023

View reviewed changes

Merge branch '2.x' into fix-spring-integration

f1c85a1

funky-eyes added the first-time contributor first-time contributor label Nov 11, 2023

Use the appropriate name

16481d2

xxxcrel requested a review from funky-eyes November 12, 2023 02:55

simplify code

432b7bf

Merge branch '2.x' into fix-spring-integration

ea47722

funky-eyes requested a review from slievrly November 13, 2023 03:12

get actually beanName

b699d93

xxxcrel changed the title ~~bugfix: can't integrated dubbo with spring~~ bugfix: can't integrate dubbo with spring Nov 13, 2023

funky-eyes reviewed Nov 13, 2023

View reviewed changes

funky-eyes requested review from sunrui1225, leezongjie and wt-better November 13, 2023 13:40

use beanFactoryName break loop

50f5a3b

add comment

4b97db7

funky-eyes approved these changes Nov 14, 2023

View reviewed changes

add changes and author

be75f7b

wt-better reviewed Nov 14, 2023

View reviewed changes

xxxcrel added 2 commits November 14, 2023 11:26

optimize code

dc70321

fix checkstyle

770b77d

leezongjie approved these changes Nov 14, 2023

View reviewed changes

funky-eyes merged commit abfafc3 into apache:2.x Nov 14, 2023
8 checks passed

open-digger-bot bot mentioned this pull request Nov 20, 2023

[WeeklyReport] Weekly report for seata 11/13/2023 to 11/20/2023 #6056

Closed

xxxcrel deleted the fix-spring-integration branch November 27, 2023 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: can't integrate dubbo with spring #6015

bugfix: can't integrate dubbo with spring #6015

xxxcrel commented Nov 10, 2023 •

edited by funky-eyes

Loading

CLAassistant commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

xxxcrel commented Nov 12, 2023 •

edited

Loading

xxxcrel commented Nov 12, 2023

funky-eyes left a comment

funky-eyes Nov 12, 2023

xxxcrel commented Nov 13, 2023

funky-eyes left a comment

wt-better Nov 14, 2023

xxxcrel Nov 14, 2023

wt-better Nov 14, 2023

xxxcrel Nov 14, 2023

wt-better Nov 14, 2023 •

edited

Loading

xxxcrel Nov 14, 2023

leezongjie left a comment

wt-better commented Nov 14, 2023

bugfix: can't integrate dubbo with spring #6015

bugfix: can't integrate dubbo with spring #6015

Conversation

xxxcrel commented Nov 10, 2023 • edited by funky-eyes Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

CLAassistant commented Nov 10, 2023 • edited Loading

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

xxxcrel commented Nov 12, 2023 • edited Loading

xxxcrel commented Nov 12, 2023

funky-eyes left a comment

Choose a reason for hiding this comment

funky-eyes Nov 12, 2023

Choose a reason for hiding this comment

xxxcrel commented Nov 13, 2023

funky-eyes left a comment

Choose a reason for hiding this comment

wt-better Nov 14, 2023

Choose a reason for hiding this comment

xxxcrel Nov 14, 2023

Choose a reason for hiding this comment

wt-better Nov 14, 2023

Choose a reason for hiding this comment

xxxcrel Nov 14, 2023

Choose a reason for hiding this comment

wt-better Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

xxxcrel Nov 14, 2023

Choose a reason for hiding this comment

leezongjie left a comment

Choose a reason for hiding this comment

wt-better commented Nov 14, 2023

xxxcrel commented Nov 10, 2023 •

edited by funky-eyes

Loading

CLAassistant commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

xxxcrel commented Nov 12, 2023 •

edited

Loading

wt-better Nov 14, 2023 •

edited

Loading