From cf499c8b5db8d1b1f6f8f0e3ae8c8a9221bce73f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 1 Apr 2019 11:04:04 +0100 Subject: [PATCH 1/2] Add mutex to loggerPromise to prevent races --- pkg/log/deleg.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/pkg/log/deleg.go b/pkg/log/deleg.go index cb711696de..c6af8ac6ee 100644 --- a/pkg/log/deleg.go +++ b/pkg/log/deleg.go @@ -17,6 +17,8 @@ limitations under the License. package log import ( + "sync" + "github.com/go-logr/logr" ) @@ -25,6 +27,7 @@ import ( type loggerPromise struct { logger *DelegatingLogger childPromises []*loggerPromise + promisesLock *sync.Mutex name *string tags []interface{} @@ -33,9 +36,13 @@ type loggerPromise struct { // WithName provides a new Logger with the name appended func (p *loggerPromise) WithName(l *DelegatingLogger, name string) *loggerPromise { res := &loggerPromise{ - logger: l, - name: &name, + logger: l, + name: &name, + promisesLock: &sync.Mutex{}, } + + p.promisesLock.Lock() + defer p.promisesLock.Unlock() p.childPromises = append(p.childPromises, res) return res } @@ -43,9 +50,13 @@ func (p *loggerPromise) WithName(l *DelegatingLogger, name string) *loggerPromis // WithValues provides a new Logger with the tags appended func (p *loggerPromise) WithValues(l *DelegatingLogger, tags ...interface{}) *loggerPromise { res := &loggerPromise{ - logger: l, - tags: tags, + logger: l, + tags: tags, + promisesLock: &sync.Mutex{}, } + + p.promisesLock.Lock() + defer p.promisesLock.Unlock() p.childPromises = append(p.childPromises, res) return res } @@ -119,7 +130,7 @@ func (l *DelegatingLogger) Fulfill(actual logr.Logger) { func NewDelegatingLogger(initial logr.Logger) *DelegatingLogger { l := &DelegatingLogger{ Logger: initial, - promise: &loggerPromise{}, + promise: &loggerPromise{promisesLock: &sync.Mutex{}}, } l.promise.logger = l return l From 29b471a2381c8b792d03e7d38d0da3208600254a Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 9 Apr 2019 10:06:04 +0100 Subject: [PATCH 2/2] Don't use pointer for sync.Mutex --- pkg/log/deleg.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/log/deleg.go b/pkg/log/deleg.go index c6af8ac6ee..949117f6bb 100644 --- a/pkg/log/deleg.go +++ b/pkg/log/deleg.go @@ -27,7 +27,7 @@ import ( type loggerPromise struct { logger *DelegatingLogger childPromises []*loggerPromise - promisesLock *sync.Mutex + promisesLock sync.Mutex name *string tags []interface{} @@ -38,7 +38,7 @@ func (p *loggerPromise) WithName(l *DelegatingLogger, name string) *loggerPromis res := &loggerPromise{ logger: l, name: &name, - promisesLock: &sync.Mutex{}, + promisesLock: sync.Mutex{}, } p.promisesLock.Lock() @@ -52,7 +52,7 @@ func (p *loggerPromise) WithValues(l *DelegatingLogger, tags ...interface{}) *lo res := &loggerPromise{ logger: l, tags: tags, - promisesLock: &sync.Mutex{}, + promisesLock: sync.Mutex{}, } p.promisesLock.Lock() @@ -130,7 +130,7 @@ func (l *DelegatingLogger) Fulfill(actual logr.Logger) { func NewDelegatingLogger(initial logr.Logger) *DelegatingLogger { l := &DelegatingLogger{ Logger: initial, - promise: &loggerPromise{promisesLock: &sync.Mutex{}}, + promise: &loggerPromise{promisesLock: sync.Mutex{}}, } l.promise.logger = l return l