Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions admiral/pkg/controller/common/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ func (s *SidecarEgressMap) Put(identity string, namespace string, fqdn string, c
}

func (s *SidecarEgressMap) Get(key string) map[string]SidecarEgress {
defer s.mutex.Unlock()
s.mutex.Lock()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
s.mutex.Lock()
s.mutex.Lock()
defer s.mutex.Unlock()

return s.cache[key]
}

Expand All @@ -186,6 +188,18 @@ func (s *SidecarEgressMap) Delete(key string) {
delete(s.cache, key)
}

// Map func returns a map of identity to namespace:SidecarEgress map
// Iterating through the returned map is not implicitly thread safe,
// use (s *SidecarEgressMap) Range() func instead.
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)) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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 ?

Copy link
Collaborator Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

defer s.mutex.Unlock()
s.mutex.Lock()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer s.mutex.Unlock

for k, v := range s.cache {
fn(k, v)
}
}
57 changes: 57 additions & 0 deletions admiral/pkg/controller/common/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package common
import (
"context"
"strings"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -191,3 +192,59 @@ func TestMapRange(t *testing.T) {
assert.Equal(t, 3, numOfIter)

}

func TestSidecarEgressGet(t *testing.T) {

egressMap := NewSidecarEgressMap()
egressMap.Put("pkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})

ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(3*time.Second))
defer cancel()

var wg sync.WaitGroup
wg.Add(2)
// Producer go routine
go func(ctx context.Context) {
nirvanagit marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Collaborator Author

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.

Copy link

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

Copy link
Collaborator Author

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.

defer wg.Done()
for {
select {
case <-ctx.Done():
return
default:
egressMap.Put("pkey1", string(uuid.NewUUID()), "fqdn", map[string]string{"pkey2": "pkey2"})
}
}
}(ctx)

// Consumer go routine
go func(ctx context.Context) {
defer wg.Done()
for {
select {
case <-ctx.Done():
return
default:
assert.NotNil(t, egressMap.Get("pkey1"))
}
}
}(ctx)

wg.Wait()
}

func TestSidecarEgressRange(t *testing.T) {

egressMap := NewSidecarEgressMap()
egressMap.Put("pkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey2", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey3", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})

numOfIter := 0
egressMap.Range(func(k string, v map[string]SidecarEgress) {
assert.NotNil(t, v)
numOfIter++
})

assert.Equal(t, 3, numOfIter)

}