wangchenxuya opened a new pull request, #1163: URL: https://github.com/apache/incubator-seata-go/pull/1163
- [x] I have registered the PR [changes](https://github.com/apache/incubator-seata-go/tree/master/changes). **What this PR does**: A synchronous request that times out removed its entry from `mergeMsgMap`, but the future had been stored in `futures`, so nothing ever removed it. Every timeout keeps one future and its request message for the lifetime of the process. `SendAsyncResponse` leaves one behind too: it sends with no callback, and the TC does not reply to a reply, so nothing ever comes back to clear it. The waiter now owns the cleanup. `syncCallback` removes the future on both of its exits, and `sendAsync` removes it on the paths that end there: an encode failure, a send failure, and a send with no callback to wait for it. The callback path is left to `syncCallback`, because an asynchronous callback returns straight away and the wait happens in the goroutine it starts. The delete is idempotent, so it does not conflict with the response processors that already remove a future once a reply arrives. **Which issue(s) this PR fixes**: Fixes #1159 **Special notes for your reviewer**: **What is and is not actually leaking.** The issue lists the timeout path. While checking it I mapped every exit so the ownership rule would be complete, and two of my first assumptions turned out to be wrong, so the table below is what the code actually does: | path | leaked before | note | |---|---|---| | timeout, no response | yes | nothing removed it; this is the reported bug | | `SendAsyncResponse` | yes | no callback waits, and no reply ever arrives | | heartbeat transfer | only while a pong is missing | its processor already cleared the future on the pong; the change makes that unconditional | | answered request | no | the on-response processor removes it; the waiter now does too, idempotently | | send failure | no | already removed | | encode failure | it panicked before reaching a return | see below | Removing the `mergeMsgMap` delete from `syncCallback` loses nothing: no production code stores into that map. **Two related defects on the same paths.** - `Encode` asserted `msg.Body.(proto.Message)` without the comma-ok form, so a body of the wrong type panicked inside `sendAsync` with the future already stored, rather than returning an error its only caller already handles. - gRPC `NotifyRpcMessageResponse` sent on `MessageFuture.Done` with no `default` case. `Done` carries a single buffered signal, so a duplicate or late response blocked the transport receive loop permanently; getty already guarded this. Both merged-response branches of the client response processor have the same unguarded send. **Those two are unreachable today**, since nothing populates `mergeMsgMap`, and are changed only for consistency. All four completion sites now go through `MessageFuture.Complete`, which records the response, wakes the waiter and reports whether it signaled. Both are outside the letter of the issue. The first is what makes the "encode failure" exit reachable as an error rather than a crash; the second is the same defect the issue reports for gRPC. Happy to split either out if you would rather review them separately. **Tests.** Every behavioural test fails on master: timeout retention, retention after a callback-less send, 200 timeouts leaving 200 entries, the gRPC duplicate response blocking for the full test deadline, and the encode panic. `TestSendSyncTimeoutLeavesNoFuture` drives the real entry point rather than `syncCallback` directly, and leaves 50 entries behind without the fix. `MessageFuture.Complete` is tested for both the first response and a dropped duplicate. **Two things I did not do, stated up front.** - `MessageFuture.Complete` still assigns `Response` before discovering the signal was dropped, so a late duplicate overwrites a response the waiter may be reading. This matches getty's existing behaviour and is not made worse here; fixing it properly needs a `sync.Once` on the shared struct, which felt like a separate change. - No test drives the merged-response path itself. The blocking logic now lives in `MessageFuture.Complete` and is tested there, but seeding a future from the processor package would need a new exported setter. **An unrelated race you may hit locally.** Running `go test ./pkg/remoting/... -race` trips a pre-existing data race roughly one run in nine on my machine: `StartReceiveLoop` spawns a per-message goroutine at `listener.go:139` that `Channel.wg` does not track, so it can outlive its test and read `grpcClientHandler.typeMap` while `e2e_test.go` writes it. Neither file is touched here. I can open a separate issue for it if that is useful. **Timing.** `RpcRequestTimeout` keeps its value; the clients read an unexported variable initialised from it so tests no longer sit on the real 20s deadline. `Test_syncCallback` drops from 20s to milliseconds, and `pkg/remoting/grpc` goes from about 143s to about 124s. The remaining 120s is `TestGrpcRemoting_SendSync_NoChannel` and `TestGrpcRemoting_SendAsync_NoChannel`, which wait on channel acquisition rather than this timeout and are untouched. Verified with Go 1.20.14 and `-race`, plus `go vet`, `gofmt` and golangci-lint v1.51.0. **Does this PR introduce a user-facing change?**: Yes, for anyone calling `Encode` directly: a body that is not a `proto.Message` now returns an error instead of panicking. ```release-note Fix message futures leaking on synchronous request timeouts and on messages sent with no callback waiting, and stop a duplicate or late response from blocking the transport receive loop. ``` -- 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]
