Repository: prometheus/tsdb
Pull request #576 (2019-04-02 11:54:29)

index: add a test to trigger fd leak on corrupted index
The test is expected to fail on Win platform only, as *nix platform
allow to remove currently opened files.

pborzenkov: As expected, CI failed on Win. Gonna add a patch that fixes the leak.

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 787a180..c154955 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,9 +1,11 @@
 ## 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
+ - [BUGFIX] Fix fd and vm_area leak on error path in chunks.NewDirReader.
+ - [BUGFIX] Fix fd and vm_area leak on error path in index.NewFileReader.
 
 ## 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 2d93d7c..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.
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..9e5cecc 100644
--- a/index/index.go
+++ b/index/index.go
@@ -29,6 +29,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/prometheus/tsdb/chunks"
 	"github.com/prometheus/tsdb/encoding"
+	tsdb_errors "github.com/prometheus/tsdb/errors"
 	"github.com/prometheus/tsdb/fileutil"
 	"github.com/prometheus/tsdb/labels"
 )
@@ -192,7 +193,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 +555,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()
@@ -625,7 +626,15 @@ func NewFileReader(path string) (*Reader, error) {
 	if err != nil {
 		return nil, err
 	}
-	return newReader(realByteSlice(f.Bytes()), f)
+	r, err := newReader(realByteSlice(f.Bytes()), f)
+	if err != nil {
+		var merr tsdb_errors.MultiError
+		merr.Add(err)
+		merr.Add(f.Close())
+		return nil, merr
+	}
+
+	return r, nil
 }
 
 func newReader(b ByteSlice, c io.Closer) (*Reader, error) {
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) {}
diff --git a/index/index_test.go b/index/index_test.go
index 70aa097..b82d07d 100644
--- a/index/index_test.go
+++ b/index/index_test.go
@@ -412,3 +412,18 @@ func TestReaderWithInvalidBuffer(t *testing.T) {
 	_, err := NewReader(b)
 	testutil.NotOk(t, err)
 }
+
+// TestNewFileReaderErrorNoOpenFiles ensures that in case of an error no file remains open.
+func TestNewFileReaderErrorNoOpenFiles(t *testing.T) {
+	dir := testutil.NewTemporaryDirectory("block", t)
+
+	idxName := filepath.Join(dir.Path(), "index")
+	err := ioutil.WriteFile(idxName, []byte("corrupted contents"), 0644)
+	testutil.Ok(t, err)
+
+	_, err = NewFileReader(idxName)
+	testutil.NotOk(t, err)
+
+	// dir.Close will fail on Win if idxName fd is not closed on error path.
+	dir.Close()
+}