From 9e61a39e216ee8b356cef2dbfb0e7872a9292e74 Mon Sep 17 00:00:00 2001
From: Abhinav Gupta <abg@uber.com>
Date: Fri, 29 Apr 2022 10:17:02 -0700
Subject: [PATCH] CheckWriteHook: Don't allow Noop action (#1089)

In #1088, we deprecated the CheckWriteAction enum in favor of the new
fully customizable CheckWriteHook. Unfortunately, this introduced a
minor regression: it's now possible to set log.Fatal to no-op with
WriteThenNoop.

Such a configuration will break code that looks like the following:

```go
f, err := os.Open(..)
if err != nil {
    log.Fatal("Cannot open file", zap.Error(err))
}

// Control flow expects that if we get here,
// f is valid and non-nil.
// That's not the case if Fatal no-ops.

fmt.Println(f.Name())
```

This change fixes the regression by turning Noops into WriteThenFatal
when logging Fatal log messages. This matches the old behavior.

It further clarifies the documentation for CheckWriteHook so that users
know to call runtime.Goexit or os.Exit in them. It's still possible to
write a custom hook that no-ops the log.Fatal, but it requires a little
extra effort from the user to do so.
---
 logger.go        | 15 ++++++++++++---
 logger_test.go   | 11 +++++++++++
 options.go       | 12 ++++++++++++
 zapcore/entry.go | 20 +++++++++++++++++---
 4 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/logger.go b/logger.go
index e04de7cb3..d87f1f0ee 100644
--- a/logger.go
+++ b/logger.go
@@ -288,9 +288,18 @@ func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry {
 		ce = ce.Should(ent, zapcore.WriteThenPanic)
 	case zapcore.FatalLevel:
 		onFatal := log.onFatal
-		// nil is the default value for CheckWriteAction, and it leads to
-		// continued execution after a Fatal which is unexpected.
-		if onFatal == nil {
+		// nil or WriteThenNoop will lead to continued execution after
+		// a Fatal log entry, which is unexpected. For example,
+		//
+		//   f, err := os.Open(..)
+		//   if err != nil {
+		//     log.Fatal("cannot open", zap.Error(err))
+		//   }
+		//   fmt.Println(f.Name())
+		//
+		// The f.Name() will panic if we continue execution after the
+		// log.Fatal.
+		if onFatal == nil || onFatal == zapcore.WriteThenNoop {
 			onFatal = zapcore.WriteThenFatal
 		}
 		ce = ce.After(ent, onFatal)
diff --git a/logger_test.go b/logger_test.go
index 50f8c6599..c71306c7d 100644
--- a/logger_test.go
+++ b/logger_test.go
@@ -535,6 +535,17 @@ func TestLoggerConcurrent(t *testing.T) {
 	})
 }
 
+func TestLoggerFatalOnNoop(t *testing.T) {
+	exitStub := exit.Stub()
+	defer exitStub.Unstub()
+	core, _ := observer.New(InfoLevel)
+
+	// We don't allow a no-op fatal hook.
+	New(core, WithFatalHook(zapcore.WriteThenNoop)).Fatal("great sadness")
+	assert.True(t, exitStub.Exited, "must exit for WriteThenNoop")
+	assert.Equal(t, 1, exitStub.Code, "must exit with status 1 for WriteThenNoop")
+}
+
 func TestLoggerCustomOnFatal(t *testing.T) {
 	tests := []struct {
 		msg          string
diff --git a/options.go b/options.go
index d6c3449ca..e2028df70 100644
--- a/options.go
+++ b/options.go
@@ -139,6 +139,18 @@ func OnFatal(action zapcore.CheckWriteAction) Option {
 }
 
 // WithFatalHook sets a CheckWriteHook to run on fatal logs.
+// Zap will call this hook after writing a log statement with a Fatal level.
+//
+// For example, the following builds a logger that will exit the current
+// goroutine after writing a fatal log message, but it will not exit the
+// program.
+//
+//   zap.New(core, zap.WithFatalHook(zapcore.WriteThenGoexit))
+//
+// It is important that the provided CheckWriteHook stops the control flow at
+// the current statement to meet expectations of callers of the logger.
+// We recommend calling os.Exit or runtime.Goexit inside custom hooks at
+// minimum.
 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 b035915f1..d91efe831 100644
--- a/zapcore/entry.go
+++ b/zapcore/entry.go
@@ -151,10 +151,24 @@ type Entry struct {
 	Stack      string
 }
 
-// CheckWriteHook allows to customize the action to take after a Fatal log entry
-// is processed.
+// CheckWriteHook is a custom action that may be executed after an entry is
+// written.
+//
+// Register one on a CheckedEntry with the After method.
+//
+//  if ce := logger.Check(...); ce != nil {
+//    ce = ce.After(hook)
+//    ce.Write(...)
+//  }
+//
+// You can configure the hook for Fatal log statements at the logger level with
+// the zap.WithFatalHook option.
 type CheckWriteHook interface {
-	// OnWrite gets invoked when an entry is written
+	// OnWrite is invoked with the CheckedEntry that was written and a list
+	// of fields added with that entry.
+	//
+	// The list of fields DOES NOT include fields that were already added
+	// to the logger with the With method.
 	OnWrite(*CheckedEntry, []Field)
 }