fix: both atelet and ate-apiserver leak gRPC connections#480
fix: both atelet and ate-apiserver leak gRPC connections#480Eric Bishop (ericdbishop) wants to merge 4 commits into
Conversation
| if ok { | ||
| conn.Close() | ||
| } | ||
| }), |
There was a problem hiding this comment.
Assuming this looks ok, not sure if there is a common util file I could move this function to so it's not duplicated across files.
There was a problem hiding this comment.
If we blindly close on eviction, can this close a connection which is already in use?
There was a problem hiding this comment.
Yes, how long lived are these connections? Maybe a delayed close might help? I also was considering if we should make the cache size configurable for both dialers.
Keith Mattix II (keithmattix)
left a comment
Keith Mattix II (keithmattix)
left a comment
There was a problem hiding this comment.
Would be nice to have a test to confirm that the connections don't leak anymore
| ateletConns: lru.New(1024), | ||
| ateletConns: lru.NewWithEvictionFunc(1024, func(key lru.Key, value any) { | ||
| // Close connection when evicting from cache. | ||
| conn, ok := value.(*grpc.ClientConn) |
There was a problem hiding this comment.
nit: should we log when either the assertion or Close() fails?
| return out | ||
| } | ||
|
|
||
| // AteomDialer handles gRPC connections to Ateom pods. |
There was a problem hiding this comment.
Do we really need this comment?
There was a problem hiding this comment.
I had just seen a similar comment in the ateapi file: https://github.com/ericdbishop/substrate/blob/5b11a10073e97f0c425bb7bee23955b2d5762dc3/cmd/ateapi/internal/controlapi/dialer.go#L31. I could remove it if preferred.
There was a problem hiding this comment.
No worries just curious
Signed-off-by: Eric Bishop <ericbish.dev@gmail.com>
Signed-off-by: Eric Bishop <ericbish.dev@gmail.com>
Signed-off-by: Eric Bishop <ericbish.dev@gmail.com>
3 participants
Fixes #470
Gen AI disclosure: Just used for adversarial review.