Ethan-Xingyue commented on PR #1164:
URL:
https://github.com/apache/incubator-seata-go/pull/1164#issuecomment-5528815052
Thanks for picking this up, @XiaoFeiASK. The direction is right: all four
paths now always reply to the TC and only propagate an error when the reply
itself fails, which matches Java's
`AbstractExceptionHandler.exceptionHandleTemplate`. Build, vet, gofmt and the
new tests pass locally on `6373ddd` with the same flags CI uses (`go test
-race`, no `-gcflags`).
I have to request changes because of one blocking issue: over the
seata/Getty protocol, the failure response this PR starts sending is not
decodable by the Java TC, and the second commit (`6373ddd`) moves in the wrong
direction.
### Blocking: `Msg` length prefix of failed branch responses does not match
the Java wire format
Java writes the `Msg` of a failed `AbstractResultMessage` with a **16-bit**
length prefix. `AbstractResultMessageCodec.encode` does
`writeByte(resultCode.ordinal())`, then on `Failed` `writeShort((short)
bs.length)` + bytes, truncated at `Short.MAX_VALUE`; `decode` reads `short len
= in.getShort()`. It is identical in the `2.x` branch and in the `v1.8.0` and
`v1.5.2` tags, so this is long-standing wire format.
On the Go side the response codecs disagree with each other:
| prefix | codecs |
| --- | --- |
| 16-bit (matches Java) | `global_begin_response_codec.go`,
`common_global_end_response_codec.go`, `branch_register_response_codec.go` |
| 8-bit (does not match) | `branch_commit_response_codec.go`,
`branch_rollback_response_codec.go`, `branch_statue_report_response_codec.go`,
`global_lock_query_resp_codec.go` |
This never fired in the RM -> TC direction before: success responses carry
no `Msg`, and on failure the old code returned before sending anything. After
this PR, every branch failure over Getty sends an 8-bit-prefixed `Msg` to the
TC.
Reproduction on this branch. Encode a failed `BranchRollbackResponse`
(`Xid="192.168.0.1:8091:1234"`, `BranchId=5678`,
`BranchStatus=PhasetwoRollbackFailedRetryable`,
`TransactionErrorCode=BranchRollbackFailedRetriable`, `ResultCode=Failed`,
`Msg="storage failed"`) and replay the Java read sequence over the bytes:
```
go encoded 49 bytes: 00 0e 73 74 6f 72 61 67 65 20 66 61 69 6c 65 64 04 00
15 31 39 32 2e 31 36 38 2e 30 2e 31 3a 38 30 39 31 3a 31 32 33 34 00 00 00 00
00 00 16 2e 09
java: resultCode=0
java: getShort() msg len=3699, bytes remaining=46 ->
BufferUnderflowException
```
`BranchCommitResponseCodec` fails the same way. As a control,
`GlobalBeginResponseCodec` (16-bit) under the same replay yields `len=14
msg="storage failed"`.
<details><summary>Replay test used (temporary <code>_test.go</code> in
<code>pkg/protocol/codec</code>)</summary>
```go
// imports: encoding/binary, testing, protocol/branch, protocol/message,
serror "util/errors"
func javaReadFailedBranchEnd(t *testing.T, b []byte) {
if b[0] != 0 { // ResultCode.Failed
t.Fatalf("expected Failed")
}
pos := 1
l := int(int16(binary.BigEndian.Uint16(b[pos:]))) // Java: in.getShort()
pos += 2
t.Logf("java: getShort() msg len=%d, bytes remaining=%d", l, len(b)-pos)
if l > len(b)-pos {
t.Fatalf("java: BufferUnderflowException")
}
}
func TestWire_JavaReadsGoFailedRollbackResponse(t *testing.T) {
msg := message.BranchRollbackResponse{AbstractBranchEndResponse:
message.AbstractBranchEndResponse{
Xid: "192.168.0.1:8091:1234", BranchId: 5678, BranchStatus:
branch.BranchStatusPhasetwoRollbackFailedRetryable,
AbstractTransactionResponse:
message.AbstractTransactionResponse{
TransactionErrorCode:
serror.TransactionErrorCodeBranchRollbackFailedRetriable,
AbstractResultMessage:
message.AbstractResultMessage{ResultCode: message.ResultCodeFailed, Msg:
"storage failed"},
},
}}
b := (&BranchRollbackResponseCodec{}).Encode(msg)
t.Logf("go encoded %d bytes: % x", len(b), b)
javaReadFailedBranchEnd(t, b)
}
```
</details>
What the TC does with it (2.x sources): `ProtocolDecoderV1.decode`
deserializes the body inside the Netty decoder and rethrows as
`DecodeException`; `AbstractNettyRemotingServer.ServerHandler.exceptionCaught`
always runs `ChannelManager.releaseRpcContext(channel)` in its `finally`, and
`RpcContext.release()` removes the client port from every resource's
`clientRMHolderMap`. So over Getty this PR does not deliver the retryable
status, and additionally makes the TC drop the RPC context of the RM channel.
The gRPC path is protobuf and unaffected.
Note that the TC only reads `BranchStatus` (`AbstractCore.branchCommitSend`
/ `branchRollbackSend` return `response.getBranchStatus()`), so the `Msg`
content is irrelevant to its decision. The problem is purely that the misparse
shifts every following field.
Please:
1. Make `branch_commit_response_codec.go` and
`branch_rollback_response_codec.go` use `bytes.WriteString16Length` /
`bytes.ReadString16Length` with `math.MaxInt16` truncation, mirroring
`global_begin_response_codec.go`. That means reverting `6373ddd` and going the
other way.
2. Rework
`TestBranchRollbackResponseCodec_TruncatesLongMessageWithoutShiftingFields`; it
currently pins the wrong 127-byte truncation.
3. Add golden-byte tests for both codecs in the style of
`TestUndoLogDeleteRequestCodec_JavaWireFormat` from #1135. For the example
above the Java bytes should be (50 bytes):
`00 | 00 0e | 73 74 6f 72 61 67 65 20 66 61 69 6c 65 64 | 04 | 00 15 | 31
39 32 2e 31 36 38 2e 30 2e 31 3a 38 30 39 31 3a 31 32 33 34 | 00 00 00 00 00 00
16 2e | 09`
The xid / branchId / branchStatus tail is confirmed against Java
`AbstractBranchEndResponseCodec`. Please double-check the single
`TransactionExceptionCode` byte against `AbstractTransactionResponseCodec`
before pinning.
4. Leave `branch_statue_report_response_codec.go` and
`global_lock_query_resp_codec.go` (TC -> RM direction, same bug) out of this
PR; they deserve a separate issue.
### Should fix
**Shared helpers placement.** `branchEndResult`, `newBranchEndResult` and
`branchEndProcessError` live in `rm_branch_commit_processor.go` L55-79 but are
used by the rollback processor too, and they sit between the
`rmBranchCommitProcessor` struct and its methods. `resourceManager()` is
duplicated verbatim in both processors. `branchEndResult` itself adds little:
it packs the old 4-line `if err != nil { ... } else { ... }` and every handler
unpacks it again. Either drop it and keep the inline mapping, or move the
genuinely shared bits into one shared file. Please don't leave helpers between
a struct and its methods.
**Injection fields.** Injection is fine here: CI runs without
`-gcflags=all=-l`, so gomonkey is unreliable, and registering a fake TCC RM
globally would leak into sibling tests. But `sendGrpcResponse` and
`sendGettyResponse` don't need to be two fields; each handler already knows its
transport, so one `sendResponse func(int32, interface{}) error` is enough. Keep
both processor structs the same shape.
**`Process()` lost coverage.** The old `TestRmBranchCommitProcessor` /
`TestRmBranchRollbackProcessor` were deleted and the new tests call
`handleGetty*` / `handleGrpc*` directly, so the protocol switch in `Process`
and the nil-fallback branches are untested (likely the 10 uncovered lines
codecov reports). One `Process`-level case per processor via
`config.InitTransportConfig` plus the injected fields would cover it.
**Contract change.** Business failure + successful send now returns `nil`
from `Process`; only send failures propagate. Both callers already discard the
return value (`pkg/remoting/getty/listener.go:100`,
`pkg/remoting/grpc/listener.go:150`), so there is no runtime impact, but please
state it in the PR description.
**End-to-end evidence.** #1156 acceptance criterion 4 asks for a run against
a real TC, and this repo has no TC in `integrate-test.yml`. Please run manually
against a 2.x seata-server over the seata protocol with a TCC `Commit` that
returns an error and paste the TC log. With the current head you should see
`Decode frame error` there, which is the finding above.
### Nits
- `pb.ResultCodeProto(result.resultCode)` relies on both enums being
`Failed=0 / Success=1`. The explicit `pb.ResultCodeProto_Failed` / `_Success`
mapping the old code had is safer.
- `log.Errorf("branch commit error: %s", ...)` and its rollback twin:
include xid and branchId while you're touching the line.
- `rm_branch_rollback_processor.go` L94 and L142: `// reply commit response
to tc server` should say rollback.
- Commit messages have no scope (`fix: ...`) while the PR title has
`fix(rm): ...`; please align (`fix(rm):`, `fix(codec):`).
- Please add the entry to `changes/dev.md` under bugfix
(`[[#1156](https://github.com/apache/incubator-seata-go/issues/1156)]`) and
tick the checkbox in the PR template.
- `// todo add TransactionErrorCode` is still there. Java sets
`TransactionExceptionCode` in `onTransactionException`, and AT's `RunUndo`
already yields a `SeataError.Code` on the Go side. Fine as a follow-up, no need
to grow this PR.
Happy to re-review once the codec fix is in.
--
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]
