Added mutex to sync access to the SidecarEgressMap #220
Conversation
| @@ -177,7 +177,10 @@ func (s *SidecarEgressMap) Put(identity string, namespace string, fqdn string, c | |||
| } | |||
|
|
|||
| func (s *SidecarEgressMap) Get(key string) map[string]SidecarEgress { | |||
| return s.cache[key] | |||
| s.mutex.Lock() | |||
There was a problem hiding this comment.
| s.mutex.Lock() | |
| s.mutex.Lock() | |
| defer s.mutex.Unlock() |
|
|
||
| // Range is a thread safe iterator to iterate through the SidecarEgress map | ||
| func (s *SidecarEgressMap) Range(fn func(k string, v map[string]SidecarEgress)) { | ||
| s.mutex.Lock() |
| func (s *SidecarEgressMap) Map() map[string]map[string]SidecarEgress { | ||
| return s.cache | ||
| } | ||
|
|
||
| // Range is a thread safe iterator to iterate through the SidecarEgress map | ||
| func (s *SidecarEgressMap) Range(fn func(k string, v map[string]SidecarEgress)) { |
There was a problem hiding this comment.
Where will this be used?
There was a problem hiding this comment.
hey @nirvanagit , I've added this func to provide a thread-safe way to iterate through the SidecarEgress map. Basically discouraging using the .Map() func to get access to the underlying map and then performing a for range on it. I've added comments to make it clear.
There was a problem hiding this comment.
Is this already being used ?
There was a problem hiding this comment.
No it's not being used right now
| func (s *SidecarEgressMap) Map() map[string]map[string]SidecarEgress { | ||
| return s.cache | ||
| } | ||
|
|
||
| // Range is a thread safe iterator to iterate through the SidecarEgress map | ||
| func (s *SidecarEgressMap) Range(fn func(k string, v map[string]SidecarEgress)) { |
Codecov Report
@@ Coverage Diff @@
## master #220 +/- ##
==========================================
+ Coverage 74.42% 74.49% +0.07%
==========================================
Files 27 27
Lines 2545 2552 +7
==========================================
+ Hits 1894 1901 +7
Misses 528 528
Partials 123 123
Continue to review full report at Codecov.
|
| ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(3*time.Second)) | ||
| defer cancel() | ||
|
|
||
| go func(ctx context.Context) { |
There was a problem hiding this comment.
| go func(ctx context.Context) { | |
| wg := synced.WaitGroup() | |
| wg.Add(1) | |
| go func(ctx context.Context) { | |
| defer wg.Done() |
Understand it is test code, but to avoid possible memory leak, waitgroup can help in this case.
There was a problem hiding this comment.
Thanks @ccfishk . I'm sorry I'm trying to understand where could this lead to a memory leak. As per my understanding, as the context's deadline expires, ctx.Done() would get called and the go routine in that case should return. Please correct me if I'm wrong.
There was a problem hiding this comment.
@shriramsharma Sorry for the late response.
I mean goroutine leak, which is memory leak, coz goroutine is not garbage cleaned by runtime object, only if the parent process specifically terminate the goroutine before it exit.
One example is that parent process stops for error while goroutine is left as it is. The clean way includes
- signal (like channle close make(chan interface{})) between parent and the routine,
- waitgroup synchronize
There was a problem hiding this comment.
@ccfishk , I think I understand where you're getting at. Thanks. The issue is with the unpredictable order in which the Done() method will be called. There could be a scenario where the parent go routine exits prior to the child.
I added Waitgroup as you suggested.
shriramsharma
force-pushed
the
concurrent_map_219
branch
from
b487b17
to
7be81eb
Compare
Added mutex to sync access to the SidecarEgressMap Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>
Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>
Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>
Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>
shriramsharma
force-pushed
the
concurrent_map_219
branch
from
6ce5e58
to
53a6b55
Compare
Signed-off-by: Shriram Sharma <shriram_sharma@intuit.com>
Signed-off-by: psikka1 <pankaj_sikka@intuit.com>
Signed-off-by: sa <sushanth_a@intuit.com>
Fixes #219
Added mutex to sync access to the SidecarEgressMap