Skip to content

Commit

Permalink
add RWlock for logger
Browse files Browse the repository at this point in the history
  • Loading branch information
jalacardio committed Aug 12, 2022
1 parent adf45b6 commit 9a44ef1
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 10 deletions.
42 changes: 35 additions & 7 deletions utils/log/loggers/fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package loggers

import (
"context"
"sync"
)

type logsContext string
Expand All @@ -12,6 +13,10 @@ var (

//LogFields contains all fields that have to be added to logs
type LogFields map[string]interface{}
type ProtectedLogFields struct {
Content LogFields

This comment has been minimized.

Copy link
@ankurs

ankurs Aug 30, 2022

Contributor

under what conditions will you add log fields in parallel ?

This comment has been minimized.

Copy link
@jalacardio

jalacardio Oct 11, 2022

Author

Hi @ankurs, sorry for responding so late. One way that I could think of is that go routine might access to the LogFields and try to add log at the same time. Another case could be, there are multiple occasion where messaging services process messages with identical context, which eventually leads to accessing same logFields simultaneously.

mtx sync.RWMutex
}

// Add or modify log fields
func (o LogFields) Add(key string, value interface{}) {
Expand All @@ -28,14 +33,19 @@ func (o LogFields) Del(key string) {
//AddToLogContext adds log fields to context.
// Any info added here will be added to all logs using this context
func AddToLogContext(ctx context.Context, key string, value interface{}) context.Context {
data := FromContext(ctx)
data := fromContext(ctx)
//Initialize if key doesn't exist
if data == nil {
ctx = context.WithValue(ctx, contextKey, make(LogFields))
data = FromContext(ctx)
ctx = context.WithValue(ctx, contextKey, &ProtectedLogFields{Content: make(LogFields)})
data = fromContext(ctx)
}
m := ctx.Value(contextKey)
if data, ok := m.(LogFields); ok {
data.Add(key, value)
if data, ok := m.(*ProtectedLogFields); ok {

This comment has been minimized.

Copy link
@ankurs

ankurs Aug 30, 2022

Contributor

you are missing a check for m.(LogFields) is someone is still using that you may end up breaking compatibility

This comment has been minimized.

Copy link
@jalacardio

jalacardio Oct 11, 2022

Author

Great concern, will need to check how context is serialized and get back to you on this.

data.mtx.Lock()
defer data.mtx.Unlock()
// d := data.Content
// fmt.Printf("Address %p\n", d)
data.Content.Add(key, value)
}
return ctx
}
Expand All @@ -46,8 +56,26 @@ func FromContext(ctx context.Context) LogFields {
return nil
}
if h := ctx.Value(contextKey); h != nil {
if logData, ok := h.(LogFields); ok {
return logData
if plf, ok := h.(*ProtectedLogFields); ok {
plf.mtx.RLock()
defer plf.mtx.RUnlock()
content := make(LogFields)
for k, v := range plf.Content {
content[k] = v
}
return content
}
}
return nil
}

func fromContext(ctx context.Context) *ProtectedLogFields {
if ctx == nil {
return nil
}
if h := ctx.Value(contextKey); h != nil {
if plf, ok := h.(*ProtectedLogFields); ok {
return plf
}
}
return nil
Expand Down
3 changes: 3 additions & 0 deletions utils/log/loggers/test/benchmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ import (

func BenchmarkFromContext(b *testing.B) {
ctx := context.Background()
for i := 0; i < 10000; i++ {
s.AddToLogContext(ctx, fmt.Sprintf("key%d", i), "good value")
}
for i := 0; i < b.N; i++ {
s.FromContext(ctx)
}
Expand Down
6 changes: 3 additions & 3 deletions utils/log/loggers/test/parallelism_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
"github.com/stretchr/testify/assert"
)

const readWorkerCount = 5
const writeWorkerCount = 5
const readWorkerCount = 50
const writeWorkerCount = 50

func readWorker(idx int, ctx context.Context) {
lf := s.FromContext(ctx)
Expand Down Expand Up @@ -64,7 +64,7 @@ func TestParallelWrite(t *testing.T) {
wg.Wait()

lf := s.FromContext(ctx)
fmt.Println(lf)
fmt.Println("lf", lf)

assert.Contains(t, lf, "test-key")
for i := 1; i <= writeWorkerCount; i++ {
Expand Down

0 comments on commit 9a44ef1

Please sign in to comment.