Occasional streamHandler map memory leak
- Dominant language
- Go
- Stars
- 664
- Forks
- 86
- PR merge metrics
- No merged PRs in 30d
Description
### Summary
I'm chasing a slow unbounded memory growth in the `containerd-shim-runc-v2`. After profiling and reviewing the code my conclusion is that there is an occasional memory leak in the ttrpc server where keys in the `streams sync.Map` are sometimes removed before the corresponding stream id key gets added (in a separate goroutine) causing the key to remain forever until connection is closed.
### Background
I'm running `containerd` on an older ARMv7 32-bit IoT device, we are currently running older 1.7 containerd branch, but the ttrpc library is fairly recent at `v1.2.7`. Our 2 core CPU is a bit on the slower side so that could perhaps explain why this leak has not been noticed before. In our setup we use containerd directly without any kubernetes/kubelet.
### Symptoms
Over a 7 days period the `containerd-shim-runc-v2` PSS_Dirty (also confirmed with Golang's `HeapAlloc`) increases from 4.2 MB to 4.6 MB.
Golangs heap pprof initially didn't initially show any leaks, but after reducing the `runtime.MemProfileRate = 1` an 8 hour diff between two heap profiles showed following:
i.e. this `streams.Store` line https://github.com/containerd/ttrpc/blob/v1.2.7/server.go#L489
### Analysis
After adding some logs to the `streams.Store` and `streams.Delete` I could see that when the `Delete` method is called it sometimes tries to remove a key that doesn't exist, when this happened then usually the deletion was also logged before the insertion.
Problems was also a bit easier to reproduce when adding an extra sleep before storing the streamHandler:
```go
diff --git a/vendor/github.com/containerd/ttrpc/server.go b/vendor/github.com/containerd/ttrpc/server.go
index bb71de677..35cfa1723 100644
--- a/vendor/github.com/containerd/ttrpc/server.go
+++ b/vendor/github.com/containerd/ttrpc/server.go
@@ -486,6 +486,7 @@ func (c *serverConn) run(sctx context.Context) {
continue
}
+ time.Sleep(1 * time.Millisecond)
streams.Store(id, sh)
atomic.AddInt32(&active, 1)
}
```
My takeaway is that there is a race where two different goroutines tries to modify the `streams` map with the same key. If the service handler runs to completion ([line 481](https://github.com/containerd/ttrpc/blob/main/server.go#L481)) before the `streams.Store` is called the key will remain forever.
### Possible solution
For the containerd use case I've only seen this race on non-streaming messages. So a potential fix is to simply skip storing the streamHandler when it is `nil`:
```go
diff --git a/vendor/github.com/containerd/ttrpc/server.go b/vendor/github.com/containerd/ttrpc/server.go
index bb71de677..9b9c0bdc7 100644
--- a/vendor/github.com/containerd/ttrpc/server.go
+++ b/vendor/github.com/containerd/ttrpc/server.go
@@ -486,7 +486,9 @@ func (c *serverConn) run(sctx context.Context) {
continue
}
- streams.Store(id, sh)
+ if sh != nil {
+ streams.Store(id, sh)
+ }
atomic.AddInt32(&active, 1)
}
// TODO: else we must ignore this for future compat. log this?
```
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.