-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added mutex to sync access to the SidecarEgressMap #220
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defer s.mutex.Unlock
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will this be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this already being used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly address comments
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>
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