diff --git a/logger.go b/logger.go index aee88fa06..e04de7cb3 100644 --- a/logger.go +++ b/logger.go @@ -288,12 +288,12 @@ func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { ce = ce.Should(ent, zapcore.WriteThenPanic) case zapcore.FatalLevel: onFatal := log.onFatal - // The nil is the default value for CheckWriteAction, and it leads to + // nil is the default value for CheckWriteAction, and it leads to // continued execution after a Fatal which is unexpected. if onFatal == nil { onFatal = zapcore.WriteThenFatal } - ce = ce.Should(ent, onFatal) + ce = ce.After(ent, onFatal) case zapcore.DPanicLevel: if log.development { ce = ce.Should(ent, zapcore.WriteThenPanic) diff --git a/logger_test.go b/logger_test.go index 497a21349..c277fb513 100644 --- a/logger_test.go +++ b/logger_test.go @@ -213,15 +213,8 @@ func TestLoggerAlwaysFatals(t *testing.T) { // Users can disable writing out fatal-level logs, but calls to logger.Fatal() // should still terminate the process. withLogger(t, FatalLevel+1, nil, func(logger *Logger, logs *observer.ObservedLogs) { - stub := exit.WithStub(func() { logger.Fatal("foo") }) + stub := exit.WithStub(func() { logger.Fatal("") }) assert.True(t, stub.Exited, "Expected calls to logger.Fatal to terminate process.") - assert.Equal(t, 1, stub.Code, "Expected calls to logger.Fatal to terminate process with predictable retcode.") - - logger = logger.WithOptions(OnFatal(zapcore.WriteThenPosixExit)) - err := errors.New("bar") - stub = exit.WithStub(func() { logger.Fatal("foo", Error(err)) }) - assert.True(t, stub.Exited, "Expected calls to logger.Fatal to terminate process.") - assert.Equal(t, 129, stub.Code, "Expected calls to logger.Fatal to terminate process with predictable retcode.") stub = exit.WithStub(func() { if ce := logger.Check(FatalLevel, ""); ce != nil { @@ -229,7 +222,6 @@ func TestLoggerAlwaysFatals(t *testing.T) { } }) assert.True(t, stub.Exited, "Expected calls to logger.Check(FatalLevel, ...) to terminate process.") - assert.Equal(t, 1, stub.Code, "Expected calls to logger.Check(FatalLevel, ...) to terminate process with predictable retcode.") assert.Equal(t, 0, logs.Len(), "Shouldn't write out logs when fatal-level logging is disabled.") }) @@ -587,6 +579,29 @@ func TestLoggerCustomOnFatal(t *testing.T) { } } +type customWriteHook struct { + called bool +} + +func (h *customWriteHook) OnWrite(_ *zapcore.CheckedEntry, _ []Field) { + h.called = true +} + +func TestLoggerWithFatalHook(t *testing.T) { + h := &customWriteHook{} + withLogger(t, InfoLevel, opts(WithFatalHook(h)), func(logger *Logger, logs *observer.ObservedLogs) { + recovered := make(chan interface{}) + go func() { + defer func() { + recovered <- recover() + }() + logger.Fatal("") + }() + <-recovered + assert.True(t, h.called) + }) +} + func TestNopLogger(t *testing.T) { logger := NewNop() diff --git a/options.go b/options.go index e65175304..d6c3449ca 100644 --- a/options.go +++ b/options.go @@ -133,12 +133,13 @@ func IncreaseLevel(lvl zapcore.LevelEnabler) Option { } // OnFatal sets the action to take on fatal logs. +// Deprecated: Use WithFatalHook instead. func OnFatal(action zapcore.CheckWriteAction) Option { - return OnFatalHook(action) + return WithFatalHook(action) } -// OnFatalHook allows to set a custom action to take on fatal logs. -func OnFatalHook(hook zapcore.CheckWriteHook) Option { +// WithFatalHook sets a CheckWriteHook to run on fatal logs. +func WithFatalHook(hook zapcore.CheckWriteHook) Option { return optionFunc(func(log *Logger) { log.onFatal = hook }) diff --git a/zapcore/entry.go b/zapcore/entry.go index 898d83e35..80c5ff2da 100644 --- a/zapcore/entry.go +++ b/zapcore/entry.go @@ -22,7 +22,6 @@ package zapcore import ( "fmt" - "hash/crc32" "runtime" "strings" "sync" @@ -155,6 +154,7 @@ type Entry struct { // CheckWriteHook allows to customize the action to take after a Fatal log entry // is processed. type CheckWriteHook interface { + // OnWrite gets invoked when an entry is written OnWrite(*CheckedEntry, []Field) } @@ -172,25 +172,26 @@ const ( WriteThenPanic // WriteThenFatal causes an os.Exit(1) after Write. WriteThenFatal - // WriteThenPosixExit causes an os.Exit(code) after Write. The exit code is - // calculated deterministically from the Fatal message, or from error Field - // if given. - WriteThenPosixExit ) -func (a CheckWriteAction) OnWrite(ce *CheckedEntry, fields []Field) { +// OnWrite implements the OnWrite method to keep CheckWriteAction compatible +// with the new CheckWriteHook interface which deprecates CheckWriteAction. +func (a CheckWriteAction) OnWrite(ce *CheckedEntry, _ []Field) { switch a { case WriteThenGoexit: runtime.Goexit() case WriteThenPanic: panic(ce.Message) case WriteThenFatal: - exit.With(1) - case WriteThenPosixExit: - exit.With(retcode(ce, fields)) + exit.Exit() } } +var _ CheckWriteHook = WriteThenNoop +var _ CheckWriteHook = WriteThenGoexit +var _ CheckWriteHook = WriteThenPanic +var _ CheckWriteHook = WriteThenFatal + // CheckedEntry is an Entry together with a collection of Cores that have // already agreed to log it. // @@ -201,7 +202,7 @@ type CheckedEntry struct { Entry ErrorOutput WriteSyncer dirty bool // best-effort detection of pool misuse - should CheckWriteHook + after CheckWriteHook cores []Core } @@ -209,9 +210,7 @@ func (ce *CheckedEntry) reset() { ce.Entry = Entry{} ce.ErrorOutput = nil ce.dirty = false - if ce.should != nil { - ce.should = WriteThenNoop - } + ce.after = nil for i := range ce.cores { // don't keep references to cores ce.cores[i] = nil @@ -249,12 +248,11 @@ func (ce *CheckedEntry) Write(fields ...Field) { ce.ErrorOutput.Sync() } - putCheckedEntry(ce) - - // Terminal operation - if ce.should != nil { - ce.should.OnWrite(ce, fields) + hook := ce.after + if hook != nil { + hook.OnWrite(ce, fields) } + putCheckedEntry(ce) } // AddCore adds a Core that has agreed to log this CheckedEntry. It's intended to be @@ -269,25 +267,22 @@ func (ce *CheckedEntry) AddCore(ent Entry, core Core) *CheckedEntry { return ce } -// Should sets this CheckedEntry's CheckWriteHook, which controls whether a +// Should sets this CheckedEntry's CheckWriteAction, which controls whether a // Core will panic or fatal after writing this log entry. Like AddCore, it's // safe to call on nil CheckedEntry references. -func (ce *CheckedEntry) Should(ent Entry, should CheckWriteHook) *CheckedEntry { +// Deprecated: Use After(ent Entry, after CheckWriteHook) instead. +func (ce *CheckedEntry) Should(ent Entry, should CheckWriteAction) *CheckedEntry { + return ce.After(ent, should) +} + +// After sets this CheckEntry's CheckWriteHook, which will be called after this +// log entry has been written. It's safe to call this on nil CheckedEntry +// references. +func (ce *CheckedEntry) After(ent Entry, hook CheckWriteHook) *CheckedEntry { if ce == nil { ce = getCheckedEntry() ce.Entry = ent } - ce.should = should + ce.after = hook return ce } - -func retcode(ce *CheckedEntry, fields []Field) int { - msg := ce.Message - for _, field := range fields { - if field.Type == ErrorType { - msg = field.Interface.(error).Error() - break - } - } - return int(crc32.ChecksumIEEE([]byte(msg)))%254 + 1 -} diff --git a/zapcore/entry_test.go b/zapcore/entry_test.go index a2242ef4b..d04cad9e7 100644 --- a/zapcore/entry_test.go +++ b/zapcore/entry_test.go @@ -64,7 +64,7 @@ func TestPutNilEntry(t *testing.T) { assert.NotNil(t, ce, "Expected only non-nil CheckedEntries in pool.") assert.False(t, ce.dirty, "Unexpected dirty bit set.") assert.Nil(t, ce.ErrorOutput, "Non-nil ErrorOutput.") - assert.Nil(t, ce.should, "Unexpected terminal behavior.") + assert.Nil(t, ce.after, "Unexpected terminal behavior.") assert.Equal(t, 0, len(ce.cores), "Expected empty slice of cores.") assert.True(t, cap(ce.cores) > 0, "Expected pooled CheckedEntries to pre-allocate slice of Cores.") } @@ -131,20 +131,10 @@ func TestCheckedEntryWrite(t *testing.T) { assert.Equal(t, 1, stub.Code, "Expected to exit when WriteThenFatal is set.") }) - t.Run("WriteThenPosixExit", func(t *testing.T) { - var ce *CheckedEntry - ce = ce.Should(Entry{Message: "foo"}, WriteThenPosixExit) - stub := exit.WithStub(func() { - ce.Write() - }) - assert.True(t, stub.Exited, "Expected to exit when WriteThenPosixExit is set.") - assert.Equal(t, 38, stub.Code, "Expected to exit with specific code when WriteThenPosixExit is set.") - }) - - t.Run("Custom", func(t *testing.T) { + t.Run("After", func(t *testing.T) { var ce *CheckedEntry hook := &customHook{} - ce = ce.Should(Entry{}, hook) + ce = ce.After(Entry{}, hook) ce.Write() assert.True(t, hook.called, "Expected to call custom action after Write.") })