Ethan-Xingyue commented on PR #1160:
URL:
https://github.com/apache/incubator-seata-go/pull/1160#issuecomment-5528865749
Thanks for the PR and the thorough write-up. I reproduced the original bug
on master: with an already-canceled context, `Commit` returns nil without ever
calling `SendSyncRequest`, and `Rollback` panics on the nil type assertion. On
this branch, `go build`, `go vet`, `gofmt` and `go mod tidy` are clean, and `go
test ./pkg/tm/... -race -count=1` passes, also with `-shuffle=on`. Letting a
received response outrank a done context makes sense to me, since
`SendSyncRequest` takes no context and a request can still complete after the
deadline.
Overall LGTM. A few non-blocking comments below.
**1. Test placement hides the transport coverage from CI**
`conformance_test.go` lives in `endphase` but exercises `getty` and `grpc`.
CI runs `go test ./... -coverprofile` without `-coverpkg`, so each test binary
only counts statements of its own package. That is why Codecov reports ~61%
patch coverage even though the new branches are exercised. Local numbers:
| | getty Commit | getty Rollback | grpc Commit | grpc Rollback |
|---|---|---|---|---|
| own-package tests (what CI measures) | 74.2% | 88.0% | 68.6% | 79.3% |
| conformance tests with `-coverpkg=./pkg/tm/transaction/...` | 83.9% | 100%
| 85.7% | 100% |
I would move the conformance cases into the existing
`getty_global_transaction_test.go` and `grpc_global_transaction_test.go`, using
the same gomonkey pattern they already have. That also avoids the `endphase`
test binary pulling in both remoting stacks to test its own callers.
**2. `errors.Is` chain for the transport error (endphase.go L58)**
`fmt.Errorf("%w, last transport error: %v", ctxErr, sendErr)` drops
`sendErr` from the `Is`/`As` chain. `go.mod` is at Go 1.20, so both can be `%w`.
**3. gomonkey claim in the test comment**
The comment above the canned-result variables says that repeatedly applying
and resetting a patch does not reliably restore the original method. The
existing getty/grpc tests do exactly that in a loop and pass. Do you have a
reference (gomonkey issue) or a reproducer? The `TestMain`-level patch is fine
either way, but as written the comment may mislead future readers.
**4. Nits**
- `Err` doc says it "returns nil only when res holds a response the caller
can use". It only checks for non-nil; the type check happens at the call site.
- `expiredContext()` calls `cancel()` right after creating an
already-expired deadline. `ctx.Err()` is still `DeadlineExceeded`, but the
extra `cancel()` reads as if the case were `Canceled`.
- `for _, m := range transportsUnderTest()` followed by `m.m.Commit(...)`:
something like `transport.manager` would read better.
- `requireNoPanic` drops the stack. Including `debug.Stack()` in the failure
message would make a real panic much easier to diagnose.
**5. Optional, same class of bug**
`Begin` in both transports still does a bare `res.(...)` assertion after the
nil check (getty L46/L52, grpc L50/L58), so a mistyped response panics there
too. Now that `endphase.UnexpectedResponse` exists, a comma-ok there is two
lines per transport. Out of scope for #1153, your call.
**Follow-up, not for this PR**
The second phase inherits the business context. Once that context is done,
neither the commit nor the compensating rollback reaches the TC, and we rely on
the TC timeout. The Java client (`DefaultGlobalTransaction.commit()` /
`DefaultTransactionManager.syncCall`) has no context and always attempts the
call. Reporting the error honestly is the right minimal fix here, but whether
the second phase should run on a detached context is probably worth a separate
issue.
--
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]
