Ethan-Xingyue opened a new issue, #1159:
URL: https://github.com/apache/incubator-seata-go/issues/1159

   
   | Field | Value |
   | --- | --- |
   | Issue title | `[BUG] [Remoting][Getty/gRPC] Timed-out synchronous requests 
leak their entries in the futures map` |
   | Labels (exist in repo) | `bug`, `remoting` |
   | Suggested priority | P1 (memory leak in long-running clients) |
   | Related | #879 (closed umbrella issue "bugs detected by doubao"); this is 
the isolated, reproducible part |
   | Verification | Reproduced 2026-09-02 on master `3bf73586` with go1.24.3 
darwin/arm64 (in-package tests, 20 s real wait each) |
   
   ---
   
   ## 🚀 Go Version
   
   go1.24.3 darwin/arm64
   
   ## 📦 Seata-go Version
   
   master, commit 3bf73586af81db1bd428982c93d82000d80cb1c8 (fetched 2026-09-02)
   
   ## 💾 Operating System
   
   macOS
   
   ## 📝 Bug Description
   
   When a synchronous RPC times out (default `RpcRequestTimeout = 20 * 
time.Second`), both `syncCallback` implementations call 
`RemoveMergedMessageFuture(reqMsg.ID)`, which deletes from `mergeMsgMap`. The 
request's `MessageFuture` however was stored in the ordinary `futures` map, so 
it is never removed. Every timeout permanently retains one future plus its 
request message.
   
   - Getty: 
https://github.com/apache/incubator-seata-go/blob/3bf73586af81db1bd428982c93d82000d80cb1c8/pkg/remoting/getty/getty_client.go#L102-L110
 (delete at L105, wrong map)
   - gRPC: 
https://github.com/apache/incubator-seata-go/blob/3bf73586af81db1bd428982c93d82000d80cb1c8/pkg/remoting/grpc/grpc_client.go#L112-L120
 (delete at L115, wrong map)
   - Futures are stored in `futures`: 
https://github.com/apache/incubator-seata-go/blob/3bf73586af81db1bd428982c93d82000d80cb1c8/pkg/remoting/grpc/grpc_remoting.go#L86-L110
 (L97)
   
   Additional ownership gaps found by reading the same code:
   
   - gRPC `sendAsync` stores the future (L97) and then returns on `Encode` 
failure (L98-L101) without deleting it; only the `channel.Send` failure path 
deletes.
   - gRPC `NotifyRpcMessageResponse` sends on `messageFuture.Done` without a 
non-blocking `select` (`grpc_remoting.go` L142). `Done` has capacity 1, so a 
duplicate or late response for the same ID would block the receive loop. Getty 
already uses a non-blocking send.
   
   Impact: a client that keeps hitting timeouts or cancellations grows the map 
linearly and keeps request/response objects alive; long-lived processes leak 
memory.
   
   ## 🔄 Steps to Reproduce
   
   1. Check out the commit:
   
   ```bash
   git clone https://github.com/apache/incubator-seata-go.git
   cd incubator-seata-go
   git checkout 3bf73586af81db1bd428982c93d82000d80cb1c8
   ```
   
   2. Save the following as `pkg/remoting/getty/audit_future_repro_test.go`:
   
   ```go
   package getty
   
   import (
        "testing"
   
        "seata.apache.org/seata-go/v2/pkg/protocol/message"
   )
   
   func TestAuditTimeoutRemovesFuture(t *testing.T) {
        client := GetGettyRemotingClient()
        req := message.RpcMessage{ID: 987654321}
        future := message.NewMessageFuture(req)
        client.gettyRemoting.futures.Store(req.ID, future)
   
        if _, err := client.syncCallback(req, future); err == nil {
                t.Fatal("expected timeout")
        }
        if client.GetMessageFuture(req.ID) != nil {
                t.Fatal("timed-out request is still retained in futures")
        }
   }
   ```
   
   3. Save the following as `pkg/remoting/grpc/audit_future_repro_test.go`:
   
   ```go
   package grpc
   
   import (
        "testing"
   
        "seata.apache.org/seata-go/v2/pkg/protocol/message"
   )
   
   func TestAuditTimeoutRemovesFuture(t *testing.T) {
        client := GetGrpcRemotingClient()
        req := message.RpcMessage{ID: 987654321}
        future := message.NewMessageFuture(req)
        client.grpcRemoting.futures.Store(req.ID, future)
   
        if _, err := client.syncCallback(req, future); err == nil {
                t.Fatal("expected timeout")
        }
        if client.GetMessageFuture(req.ID) != nil {
                t.Fatal("timed-out request is still retained in futures")
        }
   }
   ```
   
   4. Run (each package waits for the real 20 s timeout):
   
   ```bash
   go test -run '^TestAuditTimeoutRemovesFuture$' -count=1 -v \
     ./pkg/remoting/getty ./pkg/remoting/grpc
   ```
   
   Delete the two temporary test files afterwards.
   
   ## ✅ Expected Behavior
   
   After the timeout returns, `GetMessageFuture(req.ID)` is `nil`. The same 
ownership rule must hold for send errors, encode errors, context cancellation 
and connection close.
   
   ## ❌ Actual Behavior
   
   Both packages fail after about 20 seconds:
   
   ```text
   === RUN   TestAuditTimeoutRemovesFuture
   ERROR: wait resp timeout: message.RpcMessage{ID:987654321, Type:0x0, 
Codec:0x0, Compressor:0x0, HeadMap:map[string]string(nil), Body:interface 
{}(nil)}
       audit_future_repro_test.go:19: timed-out request is still retained in 
futures
   --- FAIL: TestAuditTimeoutRemovesFuture (20.00s)
   FAIL
   FAIL seata.apache.org/seata-go/v2/pkg/remoting/getty 21.273s
   === RUN   TestAuditTimeoutRemovesFuture
   ERROR: wait resp timeout: message.RpcMessage{ID:987654321, Type:0x0, 
Codec:0x0, Compressor:0x0, HeadMap:map[string]string(nil), Body:interface 
{}(nil)}
       audit_future_repro_test.go:19: timed-out request is still retained in 
futures
   --- FAIL: TestAuditTimeoutRemovesFuture (20.00s)
   FAIL
   FAIL seata.apache.org/seata-go/v2/pkg/remoting/grpc  20.655s
   FAIL
   ```
   
   ## 💡 Possible Solution
   
   - Give the future a single owner (the code that stores it) and delete 
idempotently on every exit path: success, send failure, encode failure, 
timeout, context cancel, connection close.
   - Make the timeout injectable (clock/timer) so unit tests do not wait 20 s; 
the two remoting packages currently take about 140 s each in `go test ./...` 
because of the real waits.
   - Use a non-blocking notification for duplicate/late responses in gRPC, as 
Getty does.
   
   Acceptance criteria:
   
   - [ ] The two regression tests above pass and no longer need a real 20 s 
wait.
   - [ ] success, send error, encode error, timeout, cancel, Close and 
late/duplicate response are all tested.
   - [ ] delete/complete are idempotent and `go test -race` passes.
   - [ ] After 100k timeouts/cancellations the map is back to baseline; 
goroutine and heap counts do not grow with the number of failures.


-- 
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]

Reply via email to