Merge pull request #559 from domcyrus/fix-race-on-table-file

fix race condition on table.file #557

internal/table/file.go

@@ -124,44 +124,61 @@ func (f *File) reloader() {
 	for {
 		select {
 		case <-t.C:
-			info, err := os.Stat(f.file)
+			f.reload()
-			if err != nil {
+
-				if os.IsNotExist(err) {
-					f.mLck.Lock()
-					f.m = map[string][]string{}
-					f.mStamp = time.Now()
-					f.mLck.Unlock()
-					continue
-				}
-				f.log.Error("os stat", err)
-			}
-			if info.ModTime().Before(f.mStamp) {
-				continue // reload not necessary
-			}
 		case <-f.forceReload:
+			f.reload()
+
 		case <-f.stopReloader:
 			f.stopReloader <- struct{}{}
 			return
 		}
+	}
+}
 
-		f.log.Debugf("reloading")
+func (f *File) reload() {
+	info, err := os.Stat(f.file)
+	if err != nil {
+		if os.IsNotExist(err) {
+			f.mLck.Lock()
+			f.m = map[string][]string{}
+			f.mLck.Unlock()
+			return
+		}
+		f.log.Error("os stat", err)
+	}
+	if info.ModTime().Before(f.mStamp) || time.Since(info.ModTime()) < (reloadInterval/2) {
+		return // reload not necessary
+	}
 
-		newm := make(map[string][]string, len(f.m)+5)
+	f.log.Debugf("reloading")
-		if err := readFile(f.file, newm); err != nil {
-			if os.IsNotExist(err) {
-				f.log.Printf("ignoring non-existent file: %s", f.file)
-				continue
-			}
 
-			f.log.Println(err)
+	newm := make(map[string][]string, len(f.m)+5)
-			continue
+	if err := readFile(f.file, newm); err != nil {
+		if os.IsNotExist(err) {
+			f.log.Printf("ignoring non-existent file: %s", f.file)
+			return
 		}
 
-		f.mLck.Lock()
+		f.log.Println(err)
-		f.m = newm
+		return
-		f.mStamp = time.Now()
-		f.mLck.Unlock()
 	}
+	// after reading we need to check whether file has changed in between
+	info2, err := os.Stat(f.file)
+	if err != nil {
+		f.log.Println(err)
+		return
+	}
+
+	if !info2.ModTime().Equal(info.ModTime()) {
+		// file has changed in the meantime
+		return
+	}
+
+	f.mLck.Lock()
+	f.m = newm
+	f.mStamp = info.ModTime()
+	f.mLck.Unlock()
 }
 
 func (f *File) Close() error {

internal/table/file_test.go

@@ -111,28 +111,27 @@ func TestFileReload(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// This delay is somehow important. Not sure why.
+	// ensure it is correctly loaded at first time.
-	time.Sleep(500 * time.Millisecond)
+	m.mLck.RLock()
-
+	if m.m["cat"] == nil {
-	if err := ioutil.WriteFile(f.Name(), []byte("dog: cat"), os.ModePerm); err != nil {
+		t.Fatalf("wrong content loaded, new m were not loaded, %v", m.m)
-		t.Fatal(err)
 	}
+	m.mLck.RUnlock()
 
-	for i := 0; i < 10; i++ {
+	for i := 0; i < 100; i++ {
-		time.Sleep(reloadInterval + 50*time.Millisecond)
+		// try to provoke race condition on file writing
+		if i%2 == 0 {
+			if err := os.WriteFile(f.Name(), []byte("dog: cat"), os.ModePerm); err != nil {
+				t.Fatal(err)
+			}
+		}
+		time.Sleep(reloadInterval + 5*time.Millisecond)
 		m.mLck.RLock()
-		if m.m["dog"] != nil {
+		if m.m["dog"] == nil {
-			m.mLck.RUnlock()
+			t.Fatalf("wrong content loaded, new m were not loaded, %v", m.m)
-			break
 		}
 		m.mLck.RUnlock()
 	}
-
-	m.mLck.RLock()
-	defer m.mLck.RUnlock()
-	if m.m["dog"] == nil {
-		t.Fatal("New m were not loaded")
-	}
 }
 
 func TestFileReload_Broken(t *testing.T) {
@@ -208,9 +207,6 @@ func TestFileReload_Removed(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// This delay is somehow important. Not sure why.
-	time.Sleep(250 * time.Millisecond)
-
 	os.Remove(f.Name())
 
 	time.Sleep(3 * reloadInterval)
@@ -223,5 +219,5 @@ func TestFileReload_Removed(t *testing.T) {
 }
 
 func init() {
-	reloadInterval = 250 * time.Millisecond
+	reloadInterval = 10 * time.Millisecond
 }