bugfix: fix RM_CHANNELS get NPE when resourceId is empty#5579
bugfix: fix RM_CHANNELS get NPE when resourceId is empty#5579slievrly merged 10 commits intoapache:2.xfrom
Conversation
| } | ||
|
|
||
| // ResourceId can not be null or empty | ||
| if (StringUtils.isBlank(resourceId)) { |
There was a problem hiding this comment.
是不是在服务端的rmregistry request接收处理时抛异常回去更好，因为这个问题是在seata-go发生的，在服务端处理后就可以避免其他语言都要类似加一个该判断
There was a problem hiding this comment.
@a364176773 在服务端RM注册的时候也加了校验，不过发现两个问题：
- TM 和 RM 的注册使用的
sendAsyncRequest, 是没有返回值的，所以保留了客户端的校验。 - 测试的时候发现
AbstractIdentifyResponseCodec编解码方法没有先调父类的。导致在客户端获取RegisterRMResponse和RegisterTMResponse时拿不到错误消息和请求成功结果。这个问题我也改了
funky-eyes
left a comment
funky-eyes
left a comment
There was a problem hiding this comment.
LGTM @slievrly @l81893521
funky-eyes
added
type: bug
|
|
||
| @Override | ||
| public <T> void encode(T t, ByteBuf out) { | ||
| super.encode(t, out); |
There was a problem hiding this comment.
How is it compatible with high and low versions?
low client -> current server
current client -> high server
There was a problem hiding this comment.
问题应该不在这,而在server注册的时候抛的异常,因为本身旧客户端也没有执行父类的encode,要做的应该是不同版本下对连接时,resourceid为null的处理方式进行改变
There was a problem hiding this comment.
Protocol incompatibility in the current version is not acceptable
| } | ||
|
|
||
| private static void checkDbKeySet(Set<String> dbKeySet) { | ||
| if (dbKeySet == null || dbKeySet.isEmpty()) { |
There was a problem hiding this comment.
Why throw an exception?
Registering a connection to RM and registering a DBKey are two processes.
The DBKey is allowed to be null when the connection is registered, and the DBKey can be appended to the connection later. Maybe the Datasource was not initialized when RM reconnect registered the connection.
为什么抛出异常？
RM的注册连接和注册DBKey是两个过程。
注册连接时DBKey允许为空，可以在这个连接上后续追加DBKey。因为RM reconnect注册连接时可能Datasource并未初始化完成。
There was a problem hiding this comment.
我看了代码确实是这样,先init连接,后datasource加载再发送registry,rmregistry在注册的时候会合并本地的resourceid,这块逻辑应该先去除比较好,不知道有没有好一点的办法可以提前阻断不正确的连接方式,而不是事后才知晓
There was a problem hiding this comment.
重新调整了一下代码，如果 "注册连接时DBKey允许为空" 的话，Server 端的校验就不能加上，也不能抛异常。这样的话只能在客户端去加以控制了:
- 对于
TCC Resource这种需要手工配置的resourceId，在注册时加以校验，为空直接抛出异常 - 对于其他事务的 RM 端在注册时，当校验到
resourceId为空时，打印warn日志，提示风险，待用户发现后可以进行检查 - TC 端在发起二阶段选择
channel时，先校验resourceId是否为空，是则直接返回null，避免NPE问题
There was a problem hiding this comment.
| } | ||
| RegisterTMResponse response = new RegisterTMResponse(isSuccess); | ||
| response.setIdentified(isSuccess); | ||
| if (!isSuccess) { |
There was a problem hiding this comment.
A ResultCode is a system identifier used to indicate whether the process was successful.
identified indicates the business result of the business process.
ResultCode用于表示处理是否成功，是个过程中的系统标识。
identified 表示业务处理的业务结果。
There was a problem hiding this comment.
我看在 RegisterTMResponse.class 的构造方法里，确实是直接调用的 setIdentified(isSuccess),
slievrly
changed the title
3 participants
Ⅰ. Describe what this PR did
RM注册的时候加了一个resourceId的校验，为空时抛出异常。ChannelManager#getChannel方法中，也加了一个resourceId不为空的校验，当为空时直接返回nullⅡ. Does this pull request fix one issue?
fixes #5575
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews