Open should only return close on success (#396)

Config assumes that Open will always return a close function
which causes a panic when Open returns an error, since it doesn't
return a close function.

We can instead clean up the assumption that we return partial values
on error, and instead use a more common pattern:
 - On success, `err == nil` and all other return values are valid
 - On error, `err != nil` and all other return values are zero values

Fixes #390.

config.go

@@ -217,13 +217,11 @@ func (cfg Config) buildOptions(errSink zapcore.WriteSyncer) []Option {
 func (cfg Config) openSinks() (zapcore.WriteSyncer, zapcore.WriteSyncer, error) {
 	sink, closeOut, err := Open(cfg.OutputPaths...)
 	if err != nil {
-		closeOut()
 		return nil, nil, err
 	}
-	errSink, closeErr, err := Open(cfg.ErrorOutputPaths...)
+	errSink, _, err := Open(cfg.ErrorOutputPaths...)
 	if err != nil {
 		closeOut()
-		closeErr()
 		return nil, nil, err
 	}
 	return sink, errSink, nil

config_test.go

@@ -84,3 +84,25 @@ func TestConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestConfigWithInvalidPaths(t *testing.T) {
+	tests := []struct {
+		desc      string
+		output    string
+		errOutput string
+	}{
+		{"output directory doesn't exist", "/tmp/not-there/foo.log", "stderr"},
+		{"error output directory doesn't exist", "stdout", "/tmp/not-there/foo-errors.log"},
+		{"neither output directory exists", "/tmp/not-there/foo.log", "/tmp/not-there/foo-errors.log"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			cfg := NewProductionConfig()
+			cfg.OutputPaths = []string{tt.output}
+			cfg.ErrorOutputPaths = []string{tt.errOutput}
+			_, err := cfg.Build()
+			assert.Error(t, err, "Expected an error opening a non-existent directory.")
+		})
+	}
+}

writer.go

@@ -49,6 +49,11 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
 	var errs multierror.Error
 	writers := make([]zapcore.WriteSyncer, 0, len(paths))
 	files := make([]*os.File, 0, len(paths))
+	close := func() {
+		for _, f := range files {
+			f.Close()
+		}
+	}
 	for _, path := range paths {
 		switch path {
 		case "stdout":
@@ -67,12 +72,13 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
 			files = append(files, f)
 		}
 	}
-	close := func() {
-		for _, f := range files {
-			f.Close()
-		}
+
+	if err := errs.AsError(); err != nil {
+		close()
+		return writers, nil, err
 	}
-	return writers, close, errs.AsError()
+
+	return writers, close, nil
 }
 
 // CombineWriteSyncers combines multiple WriteSyncers into a single, locked

writer_test.go

@@ -66,7 +66,10 @@ func TestOpen(t *testing.T) {
 
 	for _, tt := range tests {
 		wss, cleanup, err := open(tt.paths)
-		defer cleanup()
+		if err == nil {
+			defer cleanup()
+		}
+
 		if tt.error == "" {
 			assert.NoError(t, err, "Unexpected error opening paths %v.", tt.paths)
 		} else {
@@ -82,6 +85,25 @@ func TestOpen(t *testing.T) {
 	}
 }
 
+func TestOpenFails(t *testing.T) {
+	tests := []struct {
+		paths []string
+	}{
+		{
+			paths: []string{"./non-existent-dir/file"},
+		},
+		{
+			paths: []string{"stdout", "./non-existent-dir/file"},
+		},
+	}
+
+	for _, tt := range tests {
+		_, cleanup, err := Open(tt.paths...)
+		require.Nil(t, cleanup, "Cleanup function should never be nil")
+		assert.Error(t, err, "Open with non-existent directory should fail")
+	}
+}
+
 type testWriter struct {
 	expected string
 	t        testing.TB