You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "XiaoFeiASK (via GitHub)" <gi...@apache.org> on 2026/09/22 05:14:05 UTC
[PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
XiaoFeiASK opened a new pull request, #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195
- [ ] I have registered the PR changes.
## **What this PR does**:
Re-register cached RM resources after Getty or gRPC sessions reconnect.
Previously, a new session only sent `RegisterTMRequest`. Cached RM resources were not registered again, causing TC to lose the resource-to-RM session mapping.
This PR:
- Adds cached RM resource re-registration.
- Supports Getty session reconnect.
- Supports gRPC channel reconnect.
- Releases the session or channel when resource registration fails.
- Adds tests for multiple cached resources and registration failures.
After reconnecting, the client sends:
```
RegisterTMRequest
RegisterRMRequest{ResourceIds:"Issue128TCC"}
```
The transaction can then complete successfully without:
`rm client is not connected`
## Which issue(s) this PR fixes:
Fixes #128
## Special notes for your reviewer:
The issue was reproduced and confirmed locally. After applying this fix, the issue was verified to be resolved locally.
## Does this PR introduce a user-facing change?:
No. This change restores RM resource registration automatically after remoting session reconnection.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
Posted by "XiaoFeiASK (via GitHub)" <gi...@apache.org>.
XiaoFeiASK commented on PR #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195#issuecomment-5862261066
> The approach is right and matches the Java client (`RmNettyRemotingClient` re-registers all cached resources on every new channel). Before merging:
>
> 1. `Channel.reconnect`: don't fail the reconnect when RM re-registration fails (inline).
> 2. Nits inline: log verb, dead nil checks, shadowed variable.
> 3. Add a bugfix entry for #128 to `changes/dev.md` / `changes/dev_zh.md`.
> 4. Note in the PR description that in a multi-TC cluster the request may not reach the reconnected session (inline), and open a follow-up issue for it.
Done,and I ceate a follow-up issue for this pr
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
Posted by "XiaoFeiASK (via GitHub)" <gi...@apache.org>.
XiaoFeiASK commented on PR #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195#issuecomment-5771561515
## 重连后RM资源重注册
<img width="844" height="527" alt="ef8cae9f36bc3b485be07b54004f2116" src="https://github.com/user-attachments/assets/82599ff2-8d63-4061-9480-5526671b5f6b" />
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195#issuecomment-5771588802
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `41.37931%` with `17 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 61.37%. Comparing base ([`71728a8`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/71728a8ae36344ad522f35589a32f3b710dd4c6f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5b88933`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/5b88933a170ed0b7b7e1b0a938bef7fd0473545f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [pkg/rm/rm\_cache.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&filepath=pkg%2Frm%2Frm_cache.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3JtL3JtX2NhY2hlLmdv) | 60.00% | [5 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/remoting/grpc/channel\_manager.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&filepath=pkg%2Fremoting%2Fgrpc%2Fchannel_manager.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3JlbW90aW5nL2dycGMvY2hhbm5lbF9tYW5hZ2VyLmdv) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/remoting/getty/listener.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&filepath=pkg%2Fremoting%2Fgetty%2Flistener.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3JlbW90aW5nL2dldHR5L2xpc3RlbmVyLmdv) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/remoting/grpc/channel.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&filepath=pkg%2Fremoting%2Fgrpc%2Fchannel.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3JlbW90aW5nL2dycGMvY2hhbm5lbC5nbw==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1195 +/- ##
==========================================
- Coverage 61.37% 61.37% -0.01%
==========================================
Files 309 309
Lines 22626 22655 +29
==========================================
+ Hits 13887 13904 +17
- Misses 7446 7456 +10
- Partials 1293 1295 +2
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1195?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
Posted by "Ethan-Xingyue (via GitHub)" <gi...@apache.org>.
Ethan-Xingyue commented on code in PR #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195#discussion_r4069056821
##########
pkg/remoting/getty/listener.go:
##########
@@ -28,6 +28,7 @@ import (
"seata.apache.org/seata-go/v2/pkg/protocol/codec"
"seata.apache.org/seata-go/v2/pkg/protocol/message"
"seata.apache.org/seata-go/v2/pkg/remoting/processor"
+ "seata.apache.org/seata-go/v2/pkg/rm"
Review Comment:
This makes the transport layer depend on `pkg/rm` (also in `channel.go` / `channel_manager.go`), the reverse of the existing direction (`pkg/rm/remoting/getty` -> `pkg/remoting/getty`), and it becomes an import cycle as soon as `pkg/rm` or `pkg/tm` needs the transport layer. Non-blocking, but consider a hook (e.g. `RegisterSessionOpenHook(func() error)`) registered from `pkg/client/client.go` next to `SetRMRemotingInstance`.
##########
pkg/remoting/getty/listener.go:
##########
@@ -69,6 +70,10 @@ func (g *gettyClientHandler) OnOpen(session getty.Session) error {
sessionManager.releaseSession(session)
return
}
+ if err := rm.GetRmCacheInstance().RegisterCachedResources(); err != nil {
Review Comment:
`RegisterResource` sends via `sessionManager.selectSession`, which load-balances across the sessions of all TC servers, while TC binds resource ids to the channel the request arrives on. In a multi-TC cluster this request may therefore reach another server instead of the reconnected one. Same limitation as `RegisterTMRequest` above, so not blocking, but please note it in the PR description and open a follow-up to pin the registration to the newly opened session.
##########
pkg/rm/rm_cache_test.go:
##########
@@ -26,6 +28,45 @@ import (
"seata.apache.org/seata-go/v2/pkg/protocol/branch"
)
+func TestResourceManagerCache_RegisterCachedResources(t *testing.T) {
+ ctl := gomock.NewController(t)
+ resourceManager := NewMockResourceManager(ctl)
+ resource1 := NewMockResource(ctl)
+ resource2 := NewMockResource(ctl)
+ resources := &sync.Map{}
+ resources.Store("resource-1", resource1)
+ resources.Store("resource-2", resource2)
+
+ resourceManager.EXPECT().GetCachedResources().Return(resources)
+ resourceManager.EXPECT().RegisterResource(resource1).Return(nil)
+ resourceManager.EXPECT().RegisterResource(resource2).Return(nil)
+
+ cache := &ResourceManagerCache{}
+ cache.resourceManagerMap.Store(branch.BranchTypeTCC, resourceManager)
+
+ assert.NoError(t, cache.RegisterCachedResources())
+}
+
+func TestResourceManagerCache_RegisterCachedResourcesContinuesAfterError(t *testing.T) {
+ ctl := gomock.NewController(t)
+ resourceManager := NewMockResourceManager(ctl)
+ resource1 := NewMockResource(ctl)
+ resource2 := NewMockResource(ctl)
+ resources := &sync.Map{}
+ resources.Store("resource-1", resource1)
+ resources.Store("resource-2", resource2)
+ registrationErr := errors.New("registration failed")
+
+ resourceManager.EXPECT().GetCachedResources().Return(resources)
+ resourceManager.EXPECT().RegisterResource(resource1).Return(registrationErr)
+ resourceManager.EXPECT().RegisterResource(resource2).Return(nil)
+
+ cache := &ResourceManagerCache{}
+ cache.resourceManagerMap.Store(branch.BranchTypeTCC, resourceManager)
+
+ assert.ErrorIs(t, cache.RegisterCachedResources(), registrationErr)
+}
Review Comment:
Please also cover: `GetCachedResources()` returning nil, a non-`ResourceManager` / non-`Resource` entry, and two resource managers at once (Codecov shows 60% for this function).
##########
pkg/remoting/getty/listener.go:
##########
@@ -69,6 +70,10 @@ func (g *gettyClientHandler) OnOpen(session getty.Session) error {
sessionManager.releaseSession(session)
return
}
+ if err := rm.GetRmCacheInstance().RegisterCachedResources(); err != nil {
+ log.Errorf("register cached RM resources error: {%#v}", err)
Review Comment:
`%#v` prints the `errors.Join` result as `&errors.joinError{...}`; use `%v` (as `channel_manager.go` does).
##########
pkg/rm/rm_cache.go:
##########
@@ -59,3 +60,37 @@ func (d *ResourceManagerCache) GetResourceManager(branchType branch.BranchType)
}
return rm.(ResourceManager)
}
+
+// RegisterCachedResources re-registers all resources with their resource managers.
+// It is used after a remoting session is recreated because TC loses the old
+// resource-to-session association when the connection closes.
+func (d *ResourceManagerCache) RegisterCachedResources() error {
+ var registrationErrs []error
+
+ d.resourceManagerMap.Range(func(_, value interface{}) bool {
+ resourceManager, ok := value.(ResourceManager)
+ if !ok || resourceManager == nil {
+ registrationErrs = append(registrationErrs, fmt.Errorf("invalid resource manager cache entry: %T", value))
+ return true
+ }
+
+ resources := resourceManager.GetCachedResources()
+ if resources == nil {
+ return true
+ }
+ resources.Range(func(_, value interface{}) bool {
+ resource, ok := value.(Resource)
+ if !ok || resource == nil {
+ registrationErrs = append(registrationErrs, fmt.Errorf("invalid cached resource: %T", value))
+ return true
+ }
+ if err := resourceManager.RegisterResource(resource); err != nil {
Review Comment:
Each iteration is a synchronous round-trip with an `RpcRequestTimeout` (20s on getty) wait, so on a dead session with N resources the caller (`OnOpen` goroutine / gRPC heartbeat monitor) blocks for N x timeout. Follow-up suggestion (a TODO is fine): join all resource ids with `,` into a single `RegisterRMRequest`, like `RmNettyRemotingClient.getMergedResourceKeys()`; TC already splits `resourceIds` on `,`.
##########
pkg/remoting/grpc/channel.go:
##########
@@ -133,6 +134,9 @@ func (c *Channel) reconnect(client pb.SeataServiceClient) error {
c.registered.Store(true)
}
}
+ if err = rm.GetRmCacheInstance().RegisterCachedResources(); err != nil {
Review Comment:
Returning an error here leaves the channel half-initialized: the stream and both goroutines are already running, `hardReconnect` never closes the newly dialed `conn` (leak) and `c.conn` still points at the old connection, yet `monitorStreamHealth` re-adds the channel via `registerChannel` anyway. Suggest logging the error and continuing instead of returning; the next heartbeat round will handle a genuinely broken stream.
##########
pkg/rm/rm_cache.go:
##########
@@ -59,3 +60,37 @@ func (d *ResourceManagerCache) GetResourceManager(branchType branch.BranchType)
}
return rm.(ResourceManager)
}
+
+// RegisterCachedResources re-registers all resources with their resource managers.
+// It is used after a remoting session is recreated because TC loses the old
+// resource-to-session association when the connection closes.
+func (d *ResourceManagerCache) RegisterCachedResources() error {
+ var registrationErrs []error
+
+ d.resourceManagerMap.Range(func(_, value interface{}) bool {
+ resourceManager, ok := value.(ResourceManager)
+ if !ok || resourceManager == nil {
+ registrationErrs = append(registrationErrs, fmt.Errorf("invalid resource manager cache entry: %T", value))
+ return true
+ }
+
+ resources := resourceManager.GetCachedResources()
+ if resources == nil {
+ return true
+ }
+ resources.Range(func(_, value interface{}) bool {
Review Comment:
nit: inner `value` shadows the outer `Range` parameter; rename it.
##########
pkg/rm/rm_cache.go:
##########
@@ -59,3 +60,37 @@ func (d *ResourceManagerCache) GetResourceManager(branchType branch.BranchType)
}
return rm.(ResourceManager)
}
+
+// RegisterCachedResources re-registers all resources with their resource managers.
+// It is used after a remoting session is recreated because TC loses the old
+// resource-to-session association when the connection closes.
+func (d *ResourceManagerCache) RegisterCachedResources() error {
+ var registrationErrs []error
+
+ d.resourceManagerMap.Range(func(_, value interface{}) bool {
+ resourceManager, ok := value.(ResourceManager)
+ if !ok || resourceManager == nil {
Review Comment:
nit: `resourceManager == nil` is dead code after a successful type assertion; same for `resource == nil` on line 83.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
Posted by "XiaoFeiASK (via GitHub)" <gi...@apache.org>.
XiaoFeiASK commented on PR #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195#issuecomment-6382101934
> plz fix conflicts
ok
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
Posted by "XiaoFeiASK (via GitHub)" <gi...@apache.org>.
XiaoFeiASK commented on PR #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195#issuecomment-6382100802
plz fix conflicts
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix: re-register RM resources after session reconnect [incubator-seata-go]
Posted by "thunguo (via GitHub)" <gi...@apache.org>.
thunguo merged PR #1195:
URL: https://github.com/apache/incubator-seata-go/pull/1195
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org