wangchenxuya opened a new pull request, #1162: URL: https://github.com/apache/incubator-seata-go/pull/1162
- [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. The same map also kept an entry for every message sent without a callback, since `SendAsyncResponse` and the heartbeat transfer never wait for a reply. Both grow for the lifetime of the process. 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. 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, instead of returning an error the caller could handle. - The gRPC `NotifyRpcMessageResponse` and **both** merged-response branches of the client response processor 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. All four completion sites now go through `MessageFuture.Complete`, which records the response, wakes the waiter and reports whether it signaled. `RpcRequestTimeout` keeps its value; the clients read an unexported variable initialised from it so tests no longer sit on the real 20s deadline. **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: | path | leaked before | who clears it now | |---|---|---| | timeout, no response | yes | `syncCallback` | | send with no callback (`SendAsyncResponse`, heartbeat transfer) | yes | `sendAsync` | | answered request | no, the on-response processor removes it | `syncCallback` as well, idempotently | | heartbeat pong | no, the heartbeat processor removes it | unchanged | | send failure | no | unchanged | | encode failure | it panicked before reaching a return | `sendAsync` | **Scope.** The `Encode` comma-ok and the merged-response branches are not named in the issue. The first is what makes the "encode failure" exit reachable as an error rather than a crash, and the second is the same blocking send the issue reports for gRPC, present in the getty branch too. Both felt wrong to leave next to the fix; happy to split them out if you would rather review them separately. **Tests.** Each 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. `MessageFuture.Complete` is tested directly for both the first response and a dropped duplicate. **One coverage gap, stated plainly:** 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, which did not seem worth adding for a test. **Timing.** `Test_syncCallback` drops from 20s to milliseconds. `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 on this timeout, and are untouched here. Verified with Go 1.20.14 and `-race` over `./pkg/...`, 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]
