Repository: prometheus/tsdb
Pull request #574 (2019-04-01 09:58:05)

remove Fsync workaround for macos.
since golang 1.12 no special handling is required for file.Sync()

@pborzenkov thanks for the pointer.

Signed-off-by: Krasi Georgiev <kgeorgie@redhat.com>


Patch:
diff --git a/.travis.yml b/.travis.yml
index c146e02..6af68c3 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,8 +6,6 @@ os:
   - osx
 
 go:
-  - 1.10.x
-  - 1.11.x
   - 1.12.x
 
 go_import_path: github.com/prometheus/tsdb
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 80f5023..87bda6d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,8 +1,10 @@
 ## master / unreleased
+ - [CHANGE] tsdb now requires golang 1.12 or higher.
  - [REMOVED] `chunks.NewReader` is removed as it wasn't used anywhere.
  - [REMOVED] `FromData` is considered unused so was removed.
  - [FEATURE] Added option WALSegmentSize -1 to disable the WAL.
  - [BUGFIX] Fsync the meta file to persist it on disk to avoid data loss in case of a host crash.
+ - [BUGFIX] Fix fd and vm_area leak on error path in chunks.NewDirReader
 
 ## 0.6.1
   - [BUGFIX] Update `last` after appending a non-overlapping chunk in `chunks.MergeOverlappingChunks`. [#539](https://github.com/prometheus/tsdb/pull/539)
diff --git a/chunks/chunks.go b/chunks/chunks.go
index 9e75407..70cb119 100644
--- a/chunks/chunks.go
+++ b/chunks/chunks.go
@@ -139,7 +139,7 @@ func (w *Writer) finalizeTail() error {
 	if err := w.wbuf.Flush(); err != nil {
 		return err
 	}
-	if err := fileutil.Fsync(tf); err != nil {
+	if err := tf.Sync(); err != nil {
 		return err
 	}
 	// As the file was pre-allocated, we truncate any superfluous zero bytes.
@@ -408,13 +408,16 @@ func NewDirReader(dir string, pool chunkenc.Pool) (*Reader, error) {
 	}
 
 	var (
-		bs []ByteSlice
-		cs []io.Closer
+		bs   []ByteSlice
+		cs   []io.Closer
+		merr tsdb_errors.MultiError
 	)
 	for _, fn := range files {
 		f, err := fileutil.OpenMmapFile(fn)
 		if err != nil {
-			return nil, errors.Wrapf(err, "mmap files")
+			merr.Add(errors.Wrap(err, "mmap files"))
+			merr.Add(closeAll(cs))
+			return nil, merr
 		}
 		cs = append(cs, f)
 		bs = append(bs, realByteSlice(f.Bytes()))
@@ -422,7 +425,6 @@ func NewDirReader(dir string, pool chunkenc.Pool) (*Reader, error) {
 
 	reader, err := newReader(bs, cs, pool)
 	if err != nil {
-		var merr tsdb_errors.MultiError
 		merr.Add(err)
 		merr.Add(closeAll(cs))
 		return nil, merr
diff --git a/compact.go b/compact.go
index c8d293f..1bf33af 100644
--- a/compact.go
+++ b/compact.go
@@ -626,7 +626,7 @@ func (c *LeveledCompactor) write(dest string, meta *BlockMeta, blocks ...BlockRe
 		}
 	}()
 
-	if err := fileutil.Fsync(df); err != nil {
+	if err := df.Sync(); err != nil {
 		return errors.Wrap(err, "sync temporary dir file")
 	}
 
@@ -1026,7 +1026,7 @@ func renameFile(from, to string) error {
 		return err
 	}
 
-	if err = fileutil.Fsync(pdir); err != nil {
+	if err = pdir.Sync(); err != nil {
 		pdir.Close()
 		return err
 	}
diff --git a/db.go b/db.go
index 2ed31ea..52b21c2 100644
--- a/db.go
+++ b/db.go
@@ -37,6 +37,7 @@ import (
 	"github.com/prometheus/tsdb/chunkenc"
 	tsdb_errors "github.com/prometheus/tsdb/errors"
 	"github.com/prometheus/tsdb/fileutil"
+	_ "github.com/prometheus/tsdb/goversion"
 	"github.com/prometheus/tsdb/labels"
 	"github.com/prometheus/tsdb/wal"
 	"golang.org/x/sync/errgroup"
diff --git a/fileutil/fileutil.go b/fileutil/fileutil.go
index 154fa18..c55a2b8 100644
--- a/fileutil/fileutil.go
+++ b/fileutil/fileutil.go
@@ -118,7 +118,7 @@ func Rename(from, to string) error {
 		return err
 	}
 
-	if err = Fsync(pdir); err != nil {
+	if err = pdir.Sync(); err != nil {
 		pdir.Close()
 		return err
 	}
@@ -141,7 +141,7 @@ func Replace(from, to string) error {
 		return err
 	}
 
-	if err = Fsync(pdir); err != nil {
+	if err = pdir.Sync(); err != nil {
 		pdir.Close()
 		return err
 	}
diff --git a/fileutil/sync.go b/fileutil/sync.go
index 54dd41f..2e64a40 100644
--- a/fileutil/sync.go
+++ b/fileutil/sync.go
@@ -18,11 +18,6 @@ package fileutil
 
 import "os"
 
-// Fsync is a wrapper around file.Sync(). Special handling is needed on darwin platform.
-func Fsync(f *os.File) error {
-	return f.Sync()
-}
-
 // Fdatasync is a wrapper around file.Sync(). Special handling is needed on linux platform.
 func Fdatasync(f *os.File) error {
 	return f.Sync()
diff --git a/fileutil/sync_darwin.go b/fileutil/sync_darwin.go
index c2f39bf..2af1b0f 100644
--- a/fileutil/sync_darwin.go
+++ b/fileutil/sync_darwin.go
@@ -18,23 +18,10 @@ package fileutil
 
 import (
 	"os"
-	"syscall"
 )
 
-// Fsync on HFS/OSX flushes the data on to the physical drive but the drive
-// may not write it to the persistent media for quite sometime and it may be
-// written in out-of-order sequence. Using F_FULLFSYNC ensures that the
-// physical drive's buffer will also get flushed to the media.
-func Fsync(f *os.File) error {
-	_, _, errno := syscall.Syscall(syscall.SYS_FCNTL, f.Fd(), uintptr(syscall.F_FULLFSYNC), uintptr(0))
-	if errno == 0 {
-		return nil
-	}
-	return errno
-}
-
 // Fdatasync on darwin platform invokes fcntl(F_FULLFSYNC) for actual persistence
 // on physical drive media.
 func Fdatasync(f *os.File) error {
-	return Fsync(f)
+	return f.Sync()
 }
diff --git a/fileutil/sync_linux.go b/fileutil/sync_linux.go
index 1bbced9..8b4fc82 100644
--- a/fileutil/sync_linux.go
+++ b/fileutil/sync_linux.go
@@ -21,11 +21,6 @@ import (
 	"syscall"
 )
 
-// Fsync is a wrapper around file.Sync(). Special handling is needed on darwin platform.
-func Fsync(f *os.File) error {
-	return f.Sync()
-}
-
 // Fdatasync is similar to fsync(), but does not flush modified metadata
 // unless that metadata is needed in order to allow a subsequent data retrieval
 // to be correctly handled.
diff --git a/goversion/goversion.go b/goversion/goversion.go
new file mode 100644
index 0000000..8b194d4
--- /dev/null
+++ b/goversion/goversion.go
@@ -0,0 +1,19 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build go1.12
+
+// Package goversion enforces the go version suported by the tsdb module.
+package goversion
+
+const _SoftwareRequiresGOVERSION1_12 = uint8(0)
diff --git a/goversion/init.go b/goversion/init.go
new file mode 100644
index 0000000..dd15e1f
--- /dev/null
+++ b/goversion/init.go
@@ -0,0 +1,17 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package goversion
+
+// This will fail to compile if the Go runtime version isn't >= 1.12.
+var _ = _SoftwareRequiresGOVERSION1_12
diff --git a/index/index.go b/index/index.go
index 580291a..3f79bd7 100644
--- a/index/index.go
+++ b/index/index.go
@@ -192,7 +192,7 @@ func NewWriter(fn string) (*Writer, error) {
 	if err != nil {
 		return nil, err
 	}
-	if err := fileutil.Fsync(df); err != nil {
+	if err := df.Sync(); err != nil {
 		return nil, errors.Wrap(err, "sync dir")
 	}
 
@@ -554,7 +554,7 @@ func (w *Writer) Close() error {
 	if err := w.fbuf.Flush(); err != nil {
 		return err
 	}
-	if err := fileutil.Fsync(w.f); err != nil {
+	if err := w.f.Sync(); err != nil {
 		return err
 	}
 	return w.f.Close()
diff --git a/repair.go b/repair.go
index 4aeffb5..a7fa084 100644
--- a/repair.go
+++ b/repair.go
@@ -23,7 +23,6 @@ import (
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/pkg/errors"
-	"github.com/prometheus/tsdb/fileutil"
 )
 
 // repairBadIndexVersion repairs an issue in index and meta.json persistence introduced in
@@ -75,7 +74,7 @@ func repairBadIndexVersion(logger log.Logger, dir string) error {
 		if _, err := repl.WriteAt([]byte{2}, 4); err != nil {
 			return wrapErr(err, d)
 		}
-		if err := fileutil.Fsync(repl); err != nil {
+		if err := repl.Sync(); err != nil {
 			return wrapErr(err, d)
 		}
 		if err := repl.Close(); err != nil {
diff --git a/wal/wal.go b/wal/wal.go
index b370326..0001147 100644
--- a/wal/wal.go
+++ b/wal/wal.go
@@ -595,7 +595,7 @@ func (w *WAL) Truncate(i int) (err error) {
 
 func (w *WAL) fsync(f *Segment) error {
 	start := time.Now()
-	err := fileutil.Fsync(f.File)
+	err := f.File.Sync()
 	w.fsyncDuration.Observe(time.Since(start).Seconds())
 	return err
 }


Test patch:
diff --git a/goversion/goversio_test.go b/goversion/goversio_test.go
new file mode 100644
index 0000000..9ed3c11
--- /dev/null
+++ b/goversion/goversio_test.go
@@ -0,0 +1,27 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package goversion_test
+
+import (
+	"testing"
+
+	_ "github.com/prometheus/tsdb/goversion"
+)
+
+// This test is is intentionally blank and exists only so `go test` believes
+// there is something to test.
+//
+// The blank import above is actually what invokes the test of this package. If
+// the import succeeds (the code compiles), the test passed.
+func Test(t *testing.T) {}