Skip to content

Commit 3c5b358

Browse files
authored
Merge pull request #1339 from alvaroaleman/wtf-lograce
🐛 Fix a race in the delegating logger
2 parents 0918618 + 2aaa357 commit 3c5b358

File tree

2 files changed

+47
-0
lines changed

2 files changed

+47
-0
lines changed

pkg/log/deleg.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,10 @@ func (p *loggerPromise) Fulfill(parentLogger logr.Logger) {
7272
logger = logger.WithValues(p.tags...)
7373
}
7474

75+
p.logger.lock.Lock()
7576
p.logger.Logger = logger
7677
p.logger.promise = nil
78+
p.logger.lock.Unlock()
7779

7880
for _, childPromise := range p.childPromises {
7981
childPromise.Fulfill(logger)
@@ -86,12 +88,16 @@ func (p *loggerPromise) Fulfill(parentLogger logr.Logger) {
8688
// logger. It expects to have *some* logr.Logger set at all times (generally
8789
// a no-op logger before the promises are fulfilled).
8890
type DelegatingLogger struct {
91+
lock sync.Mutex
8992
logr.Logger
9093
promise *loggerPromise
9194
}
9295

9396
// WithName provides a new Logger with the name appended
9497
func (l *DelegatingLogger) WithName(name string) logr.Logger {
98+
l.lock.Lock()
99+
defer l.lock.Unlock()
100+
95101
if l.promise == nil {
96102
return l.Logger.WithName(name)
97103
}
@@ -105,6 +111,9 @@ func (l *DelegatingLogger) WithName(name string) logr.Logger {
105111

106112
// WithValues provides a new Logger with the tags appended
107113
func (l *DelegatingLogger) WithValues(tags ...interface{}) logr.Logger {
114+
l.lock.Lock()
115+
defer l.lock.Unlock()
116+
108117
if l.promise == nil {
109118
return l.Logger.WithValues(tags...)
110119
}

pkg/log/log_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,44 @@ var _ = Describe("logging", func() {
162162
))
163163
})
164164

165+
// This test in itself will always succeed, a failure will be indicated by the
166+
// race detector going off
167+
It("should be threadsafe", func() {
168+
fulfillDone := make(chan struct{})
169+
withNameDone := make(chan struct{})
170+
withValuesDone := make(chan struct{})
171+
grandChildDone := make(chan struct{})
172+
173+
// Constructing the child in the goroutine does not reliably
174+
// trigger the race detector
175+
child := delegLog.WithName("child")
176+
go func() {
177+
defer GinkgoRecover()
178+
delegLog.Fulfill(NullLogger{})
179+
close(fulfillDone)
180+
}()
181+
go func() {
182+
defer GinkgoRecover()
183+
delegLog.WithName("with-name")
184+
close(withNameDone)
185+
}()
186+
go func() {
187+
defer GinkgoRecover()
188+
delegLog.WithValues("with-value")
189+
close(withValuesDone)
190+
}()
191+
go func() {
192+
defer GinkgoRecover()
193+
child.WithValues("grandchild")
194+
close(grandChildDone)
195+
}()
196+
197+
<-fulfillDone
198+
<-withNameDone
199+
<-withValuesDone
200+
<-grandChildDone
201+
})
202+
165203
It("should delegate with tags", func() {
166204
By("asking for a logger with a name before fulfill, and logging")
167205
befFulfill1 := delegLog.WithValues("tag1", "val1")

0 commit comments

Comments
 (0)