From f84f3ac6e74b1929b2e2bf42512e34e52e857080 Mon Sep 17 00:00:00 2001
From: Prashant Varanasi <prashant@uber.com>
Date: Tue, 28 Mar 2017 23:31:34 -0700
Subject: [PATCH 1/2] Open should only return close on success

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
---
 config.go      |  4 +---
 config_test.go | 22 ++++++++++++++++++++++
 writer.go      | 16 +++++++++++-----
 writer_test.go | 25 ++++++++++++++++++++++++-
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/config.go b/config.go
index a198f16ea..cdd251981 100644
--- a/config.go
+++ b/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
diff --git a/config_test.go b/config_test.go
index 65bcfb112..65cabdfd7 100644
--- a/config_test.go
+++ b/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.")
+		})
+	}
+}
diff --git a/writer.go b/writer.go
index 238ca6f36..de39e5992 100644
--- a/writer.go
+++ b/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
diff --git a/writer_test.go b/writer_test.go
index 9cc3e775d..abf74c097 100644
--- a/writer_test.go
+++ b/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,26 @@ func TestOpen(t *testing.T) {
 	}
 }
 
+func TestOpenFails(t *testing.T) {
+	tests := []struct {
+		paths   []string
+		wantErr 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

From 83243b9473e79c2bdf242d6e783349d3cd33b27f Mon Sep 17 00:00:00 2001
From: Prashant Varanasi <prashant@uber.com>
Date: Wed, 29 Mar 2017 09:16:53 -0700
Subject: [PATCH 2/2] Remove unused wantErr field

---
 writer_test.go | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/writer_test.go b/writer_test.go
index abf74c097..1293832ab 100644
--- a/writer_test.go
+++ b/writer_test.go
@@ -87,8 +87,7 @@ func TestOpen(t *testing.T) {
 
 func TestOpenFails(t *testing.T) {
 	tests := []struct {
-		paths   []string
-		wantErr string
+		paths []string
 	}{
 		{
 			paths: []string{"./non-existent-dir/file"},