From 2f5b8a91be2cea93c2a6792d4ad00f5a06967dae Mon Sep 17 00:00:00 2001 From: Varun Chawla <34209028+veeceey@users.noreply.github.com> Date: Tue, 17 Mar 2026 05:52:29 -0700 Subject: [PATCH] fix: UpdateContext skips Nop and zero-value loggers (#754) UpdateContext previously used pointer equality (l == disabledLogger) to detect disabled loggers. This only caught the package-level singleton returned by Ctx() but missed loggers created via Nop() or zero-value Logger{}, which could lead to data races when a shared Nop logger was used from multiple goroutines. Replace the pointer comparison with a check for nil writer or Disabled level, consistent with how Logger.should() determines if logging is active. This prevents unnecessary context mutations on loggers that will never produce output. Fixes #643 Co-Authored-By: Claude Opus 4.6 --- log.go | 9 +++++++-- log_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/log.go b/log.go index 9ff669e..f9ba541 100644 --- a/log.go +++ b/log.go @@ -294,7 +294,7 @@ func (l Logger) With() Context { // Caution: This method is not concurrency safe. // Use the With method to create a child logger before modifying the context from concurrent goroutines. func (l *Logger) UpdateContext(update func(c Context) Context) { - if l == disabledLogger { + if l.disabled() { return } if cap(l.context) == 0 { @@ -508,9 +508,14 @@ func (l *Logger) scratchEvent() *Event { return newEvent(LevelWriterAdapter{io.Discard}, DebugLevel, l.stack, l.ctx, l.hooks) } +// disabled returns true if the logger is a disabled or nop logger. +func (l *Logger) disabled() bool { + return l.w == nil || l.level == Disabled +} + // should returns true if the log event should be logged. func (l *Logger) should(lvl Level) bool { - if l.w == nil { + if l.disabled() { return false } if lvl < l.level || lvl < GlobalLevel() { diff --git a/log_test.go b/log_test.go index 9ca35d6..99de499 100644 --- a/log_test.go +++ b/log_test.go @@ -1214,6 +1214,39 @@ func TestUpdateContextOnDisabledLogger(t *testing.T) { } } +func TestUpdateContextOnNopLogger(t *testing.T) { + // Nop() creates a new disabled logger that is a different pointer than + // the package-level disabledLogger. UpdateContext should still treat it + // as disabled and skip the update function. See issue #643. + log := Nop() + + called := false + log.UpdateContext(func(c Context) Context { + called = true + return c.Str("foo", "bar") + }) + + if called { + t.Error("UpdateContext should not call update func on a Nop logger") + } +} + +func TestUpdateContextOnZeroValueLogger(t *testing.T) { + // A zero-value Logger has a nil writer and should be treated as + // disabled by UpdateContext. See issue #643. + var log Logger + + called := false + log.UpdateContext(func(c Context) Context { + called = true + return c.Str("foo", "bar") + }) + + if called { + t.Error("UpdateContext should not call update func on a zero-value logger") + } +} + func TestLevel_String(t *testing.T) { tests := []struct { name string