From a5e8eafcb640b55db4aad5a2121e8bbd1bf04143 Mon Sep 17 00:00:00 2001
From: Goutham Veeramachaneni <gouthamve@gmail.com>
Date: Thu, 20 Sep 2018 16:09:20 +0530
Subject: [PATCH] Review feedback

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
---
 compact.go |  4 ++--
 wal.go     | 20 ++++++++++++++++----
 wal/wal.go |  5 +++++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/compact.go b/compact.go
index 6d82f3f4..ad0f918e 100644
--- a/compact.go
+++ b/compact.go
@@ -479,8 +479,8 @@ func (c *LeveledCompactor) write(dest string, meta *BlockMeta, blocks ...BlockRe
 
 	// We are explicitly closing them here to check for error even
 	// though these are covered under defer. This is because in Windows,
-	// you cannot delete these unless there is they are closed and we want to close them
-	// in case of any errors above.
+	// you cannot delete these unless they are closed and the defer is to
+	// make sure they are closed if the function exits due to an error above.
 	if err = chunkw.Close(); err != nil {
 		return errors.Wrap(err, "close chunk writer")
 	}
diff --git a/wal.go b/wal.go
index 34a87b60..59206d8d 100644
--- a/wal.go
+++ b/wal.go
@@ -723,6 +723,13 @@ func (w *SegmentWAL) run(interval time.Duration) {
 
 // Close syncs all data and closes the underlying resources.
 func (w *SegmentWAL) Close() error {
+	// Make sure you can call Close() multiple times.
+	select {
+	case <-w.stopc:
+		return nil // Already closed.
+	default:
+	}
+
 	close(w.stopc)
 	<-w.donec
 
@@ -736,7 +743,7 @@ func (w *SegmentWAL) Close() error {
 	// only the current segment will still be open.
 	if hf := w.head(); hf != nil {
 		if err := hf.Close(); err != nil {
-			return errors.Wrapf(hf.Close(), "closing WAL head %s", hf.Name())
+			return errors.Wrapf(err, "closing WAL head %s", hf.Name())
 		}
 	}
 
@@ -1262,6 +1269,7 @@ func MigrateWAL(logger log.Logger, dir string) (err error) {
 	if err != nil {
 		return errors.Wrap(err, "open new WAL")
 	}
+
 	// It should've already been closed as part of the previous finalization.
 	// Do it once again in case of prior errors.
 	defer func() {
@@ -1274,6 +1282,7 @@ func MigrateWAL(logger log.Logger, dir string) (err error) {
 	if err != nil {
 		return errors.Wrap(err, "open old WAL")
 	}
+	defer w.Close()
 
 	rdr := w.Reader()
 
@@ -1307,12 +1316,15 @@ func MigrateWAL(logger log.Logger, dir string) (err error) {
 	if err != nil {
 		return errors.Wrap(err, "write new entries")
 	}
-	if err := repl.Close(); err != nil {
-		return errors.Wrap(err, "close new WAL")
-	}
+	// We explicitly close even when there is a defer for Windows to be
+	// able to delete it. The defer is in place to close it in-case there
+	// are errors above.
 	if err := w.Close(); err != nil {
 		return errors.Wrap(err, "close old WAL")
 	}
+	if err := repl.Close(); err != nil {
+		return errors.Wrap(err, "close new WAL")
+	}
 	if err := fileutil.Replace(tmpdir, dir); err != nil {
 		return errors.Wrap(err, "replace old WAL")
 	}
diff --git a/wal/wal.go b/wal/wal.go
index 64610889..aa52738f 100644
--- a/wal/wal.go
+++ b/wal/wal.go
@@ -320,6 +320,8 @@ func (w *WAL) Repair(origErr error) error {
 	if err != nil {
 		return errors.Wrap(err, "open segment")
 	}
+	defer f.Close()
+
 	r := NewReader(bufio.NewReader(f))
 
 	for r.Next() {
@@ -329,6 +331,9 @@ func (w *WAL) Repair(origErr error) error {
 	}
 	// We expect an error here from r.Err(), so nothing to handle.
 
+	// We explicitly close even when there is a defer for Windows to be
+	// able to delete it. The defer is in place to close it in-case there
+	// are errors above.
 	if err := f.Close(); err != nil {
 		return errors.Wrap(err, "close corrupted file")
 	}