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.

Author: abhinav

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)

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

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

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)
 }