Copilot commented on code in PR #1171:
URL:
https://github.com/apache/incubator-seata-go/pull/1171#discussion_r4028718680
##########
pkg/rm/rm_cache.go:
##########
@@ -53,9 +52,18 @@ func (d *ResourceManagerCache)
UnregisterResourceManager(branchType branch.Branc
}
func (d *ResourceManagerCache) GetResourceManager(branchType
branch.BranchType) ResourceManager {
+ rm, _ := d.FindResourceManager(branchType)
+ return rm
+}
+
+func (d *ResourceManagerCache) FindResourceManager(branchType
branch.BranchType) (ResourceManager, bool) {
rm, ok := d.resourceManagerMap.Load(branchType)
if !ok {
- panic(fmt.Sprintf("No ResourceManagerCache for BranchType: %v",
branchType))
+ return nil, false
+ }
+ manager, ok := rm.(ResourceManager)
+ if !ok {
+ return nil, false
}
- return rm.(ResourceManager)
+ return manager, true
}
Review Comment:
`GetResourceManager` previously enforced correctness by panicking when the
RM was absent; it now silently returns `nil`, which increases the chance of
accidental `nil` dereferences at call sites and changes the contract without
being obvious from the name. Consider (mandatory) adding a doc comment that
explicitly states it returns `nil` when not found and directing callers to
`FindResourceManager`, or (optionally) renaming/introducing a
`MustGetResourceManager` helper if any internal paths still want fail-fast
behavior.
##########
pkg/remoting/processor/client/rm_branch_rollback_processor.go:
##########
@@ -73,7 +73,18 @@ func (f *rmBranchRollbackProcessor)
handleGrpcBranchRollback(ctx context.Context
ResourceId: resourceID,
ApplicationData: []byte(applicationData),
}
- status, err :=
rm.GetRmCacheInstance().GetResourceManager(branchType).BranchRollback(ctx,
branchResource)
+ resourceManager, ok :=
rm.GetRmCacheInstance().FindResourceManager(branchType)
+ if !ok {
+ errMsg := resourceManagerNotFoundMsg(branchType)
+ log.Errorf("branch rollback error: %s", errMsg)
+ err :=
grpc.GetGrpcRemotingClient().SendAsyncResponse(rpcMessage.ID,
newFailedGrpcBranchRollbackResponse(xid, branchID, errMsg))
Review Comment:
`resourceManagerNotFoundMsg` is defined in `rm_branch_commit_processor.go`
(same package), so rollback now has an implicit cross-file dependency for a
small helper. Consider moving shared helpers like `resourceManagerNotFoundMsg`
into a dedicated shared file (e.g., `resource_manager_helpers.go`) near both
processors to make reuse explicit and reduce the chance of accidental
divergence.
##########
pkg/rm/issue1158_repro_test.go:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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 rm
+
+import (
+ "testing"
+
+ "seata.apache.org/seata-go/v2/pkg/protocol/branch"
+)
+
+func TestUnknownBranchTypeReturnsErrorInsteadOfPanicking(t *testing.T) {
Review Comment:
The test name says \"ReturnsError\" but the assertion verifies a `nil`
return (and absence of panic), not an error. Rename the test to reflect the
actual behavior being validated (e.g., \"ReturnsNilInsteadOfPanicking\") to
keep intent clear.
##########
pkg/rm/issue1158_repro_test.go:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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 rm
+
+import (
+ "testing"
+
+ "seata.apache.org/seata-go/v2/pkg/protocol/branch"
+)
+
+func TestUnknownBranchTypeReturnsErrorInsteadOfPanicking(t *testing.T) {
+ defer func() {
+ if r := recover(); r != nil {
+ t.Fatalf("unknown branch type panicked: %v", r)
+ }
+ }()
+ if got :=
GetRmCacheInstance().GetResourceManager(branch.BranchType(99)); got != nil {
+ t.Fatalf("unexpected resource manager: %T", got)
+ }
+}
Review Comment:
The test name says \"ReturnsError\" but the assertion verifies a `nil`
return (and absence of panic), not an error. Rename the test to reflect the
actual behavior being validated (e.g., \"ReturnsNilInsteadOfPanicking\") to
keep intent clear.
##########
pkg/rm/tcc/tcc_service.go:
##########
@@ -58,7 +58,13 @@ func NewTCCServiceProxy(service interface{})
(*TCCServiceProxy, error) {
func (t *TCCServiceProxy) RegisterResource() error {
var err error
t.registerResourceOnce.Do(func() {
- err =
rm.GetRmCacheInstance().GetResourceManager(branch.BranchTypeTCC).RegisterResource(t.TCCResource)
+ mgr :=
rm.GetRmCacheInstance().GetResourceManager(branch.BranchTypeTCC)
+ if mgr == nil {
+ err = fmt.Errorf("no ResourceManager for BranchType:
%v", branch.BranchTypeTCC)
+ log.Errorf("NewTCCServiceProxy RegisterResource error:
%#v", err.Error())
+ return
+ }
+ err = mgr.RegisterResource(t.TCCResource)
if err != nil {
log.Errorf("NewTCCServiceProxy RegisterResource error:
%#v", err.Error())
Review Comment:
The log call formats a `string` with `%#v` and also logs `err.Error()`
rather than the `error` itself, which reduces clarity and can produce confusing
output. Prefer logging `err` directly with `%v` (and avoid calling `Error()`
here) so structured/typed error information is preserved.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
