Rewrite ignores to fix data race, use fewer maps

internal/ignore/cache.go

@@ -0,0 +1,95 @@
+// Copyright (C) 2014 The Syncthing Authors.
+//
+// This program is free software: you can redistribute it and/or modify it
+// under the terms of the GNU General Public License as published by the Free
+// Software Foundation, either version 3 of the License, or (at your option)
+// any later version.
+//
+// This program is distributed in the hope that it will be useful, but WITHOUT
+// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+// more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this program. If not, see <http://www.gnu.org/licenses/>.
+
+package ignore
+
+import (
+	"sync"
+	"time"
+)
+
+var (
+	caches   = make(map[string]*cache)
+	cacheMut sync.Mutex
+)
+
+func init() {
+	// Periodically go through the cache and remove cache entries that have
+	// not been touched in the last two hours.
+	go cleanIgnoreCaches(2 * time.Hour)
+}
+
+type cache struct {
+	patterns []Pattern
+	entries  map[string]cacheEntry
+	mut      sync.Mutex
+}
+
+type cacheEntry struct {
+	value  bool
+	access time.Time
+}
+
+func newCache(patterns []Pattern) *cache {
+	return &cache{
+		patterns: patterns,
+		entries:  make(map[string]cacheEntry),
+	}
+}
+
+func (c *cache) clean(d time.Duration) {
+	c.mut.Lock()
+	for k, v := range c.entries {
+		if time.Since(v.access) > d {
+			delete(c.entries, k)
+		}
+	}
+	c.mut.Unlock()
+}
+
+func (c *cache) get(key string) (result, ok bool) {
+	c.mut.Lock()
+	res, ok := c.entries[key]
+	if ok {
+		res.access = time.Now()
+		c.entries[key] = res
+	}
+	c.mut.Unlock()
+	return res.value, ok
+}
+
+func (c *cache) set(key string, val bool) {
+	c.mut.Lock()
+	c.entries[key] = cacheEntry{val, time.Now()}
+	c.mut.Unlock()
+}
+
+func (c *cache) len() int {
+	c.mut.Lock()
+	l := len(c.entries)
+	c.mut.Unlock()
+	return l
+}
+
+func cleanIgnoreCaches(dur time.Duration) {
+	for {
+		time.Sleep(dur)
+		cacheMut.Lock()
+		for _, v := range caches {
+			v.clean(dur)
+		}
+		cacheMut.Unlock()
+	}
+}

internal/ignore/cache_test.go

@@ -0,0 +1,84 @@
+// Copyright (C) 2014 The Syncthing Authors.
+//
+// This program is free software: you can redistribute it and/or modify it
+// under the terms of the GNU General Public License as published by the Free
+// Software Foundation, either version 3 of the License, or (at your option)
+// any later version.
+//
+// This program is distributed in the hope that it will be useful, but WITHOUT
+// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+// more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this program. If not, see <http://www.gnu.org/licenses/>.
+
+package ignore
+
+import (
+	"testing"
+	"time"
+)
+
+func TestCache(t *testing.T) {
+	c := newCache(nil)
+
+	res, ok := c.get("nonexistent")
+	if res != false || ok != false {
+		t.Error("res %v, ok %v for nonexistent item", res, ok)
+	}
+
+	// Set and check some items
+
+	c.set("true", true)
+	c.set("false", false)
+
+	res, ok = c.get("true")
+	if res != true || ok != true {
+		t.Errorf("res %v, ok %v for true item", res, ok)
+	}
+
+	res, ok = c.get("false")
+	if res != false || ok != true {
+		t.Errorf("res %v, ok %v for false item", res, ok)
+	}
+
+	// Don't clean anything
+
+	c.clean(time.Second)
+
+	// Same values should exist
+
+	res, ok = c.get("true")
+	if res != true || ok != true {
+		t.Errorf("res %v, ok %v for true item", res, ok)
+	}
+
+	res, ok = c.get("false")
+	if res != false || ok != true {
+		t.Errorf("res %v, ok %v for false item", res, ok)
+	}
+
+	// Sleep and access, to get some data for clean
+
+	time.Sleep(100 * time.Millisecond)
+	c.get("true")
+	time.Sleep(100 * time.Millisecond)
+
+	// "false" was accessed 200 ms ago, "true" was accessed 100 ms ago.
+	// This should clean out "false" but not "true"
+
+	c.clean(150 * time.Millisecond)
+
+	// Same values should exist
+
+	_, ok = c.get("true")
+	if !ok {
+		t.Error("item should still exist")
+	}
+
+	_, ok = c.get("false")
+	if ok {
+		t.Errorf("item should have been cleaned")
+	}
+}

internal/ignore/ignore.go

@@ -28,23 +28,15 @@ import (
 	"github.com/syncthing/syncthing/internal/fnmatch"
 )
 
-var caches = make(map[string]MatcherCache)
-
 type Pattern struct {
 	match   *regexp.Regexp
 	include bool
 }
 
 type Matcher struct {
-	patterns   []Pattern
-	oldMatches map[string]bool
-	newMatches map[string]bool
-	mut        sync.Mutex
-}
-
-type MatcherCache struct {
 	patterns []Pattern
-	matches  map[string]bool
+	matches  *cache
+	mut      sync.Mutex
 }
 
 func Load(file string, cache bool) (*Matcher, error) {
@@ -54,6 +46,9 @@ func Load(file string, cache bool) (*Matcher, error) {
 		return matcher, err
 	}
 
+	cacheMut.Lock()
+	defer cacheMut.Unlock()
+
 	// Get the current cache object for the given file
 	cached, ok := caches[file]
 	if !ok || !patternsEqual(cached.patterns, matcher.patterns) {
@@ -61,12 +56,9 @@ func Load(file string, cache bool) (*Matcher, error) {
 		// store matches for the given set of patterns.
 		// Initialize oldMatches to indicate that we are interested in
 		// caching.
-		matcher.oldMatches = make(map[string]bool)
-		matcher.newMatches = make(map[string]bool)
-		caches[file] = MatcherCache{
-			patterns: matcher.patterns,
-			matches:  matcher.newMatches,
-		}
+		cached = newCache(matcher.patterns)
+		matcher.matches = cached
+		caches[file] = cached
 		return matcher, nil
 	}
 
@@ -74,10 +66,7 @@ func Load(file string, cache bool) (*Matcher, error) {
 	// matches map and update the pointer. (This prevents matches map from
 	// growing indefinately, as we only cache whatever we've matched in the last
 	// iteration, rather than through runtime history)
-	matcher.oldMatches = cached.matches
-	matcher.newMatches = make(map[string]bool)
-	cached.matches = matcher.newMatches
-	caches[file] = cached
+	matcher.matches = cached
 	return matcher, nil
 }
 
@@ -93,27 +82,27 @@ func (m *Matcher) Match(file string) (result bool) {
 		return false
 	}
 
-	// We have old matches map set, means we should do caching
-	if m.oldMatches != nil {
-		// Capture the result to the new matches regardless of who returns it
-		defer func() {
-			m.mut.Lock()
-			m.newMatches[file] = result
-			m.mut.Unlock()
-		}()
-		// Check perhaps we've seen this file before, and we already know
-		// what the outcome is going to be.
-		result, ok := m.oldMatches[file]
+	if m.matches != nil {
+		// Check the cache for a known result.
+		res, ok := m.matches.get(file)
 		if ok {
-			return result
+			return res
 		}
+
+		// Update the cache with the result at return time
+		defer func() {
+			m.matches.set(file, result)
+		}()
 	}
 
+	// Check all the patterns for a match.
 	for _, pattern := range m.patterns {
 		if pattern.match.MatchString(file) {
 			return pattern.include
 		}
 	}
+
+	// Default to false.
 	return false
 }
 

internal/ignore/ignore_test.go

@@ -173,12 +173,8 @@ func TestCaching(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if pats.oldMatches == nil || len(pats.oldMatches) != 0 {
-		t.Fatal("Expected empty map")
-	}
-
-	if pats.newMatches == nil || len(pats.newMatches) != 0 {
-		t.Fatal("Expected empty map")
+	if pats.matches.len() != 0 {
+		t.Fatal("Expected empty cache")
 	}
 
 	if len(pats.patterns) != 4 {
@@ -191,7 +187,7 @@ func TestCaching(t *testing.T) {
 		pats.Match(letter)
 	}
 
-	if len(pats.newMatches) != 4 {
+	if pats.matches.len() != 4 {
 		t.Fatal("Expected 4 cached results")
 	}
 
@@ -201,30 +197,10 @@ func TestCaching(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(pats.oldMatches) != 4 {
+	if pats.matches.len() != 4 {
 		t.Fatal("Expected 4 cached results")
 	}
 
-	// Match less this time
-
-	for _, letter := range []string{"b", "x", "y"} {
-		pats.Match(letter)
-	}
-
-	if len(pats.newMatches) != 3 {
-		t.Fatal("Expected 3 cached results")
-	}
-
-	// Reload file, expect the new outcomes to be provided
-
-	pats, err = Load(fd1.Name(), true)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(pats.oldMatches) != 3 {
-		t.Fatal("Expected 3 cached results", len(pats.oldMatches))
-	}
-
 	// Modify the include file, expect empty cache
 
 	fd2.WriteString("/z/\n")
@@ -234,7 +210,7 @@ func TestCaching(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if len(pats.oldMatches) != 0 {
+	if pats.matches.len() != 0 {
 		t.Fatal("Expected 0 cached results")
 	}
 
@@ -250,7 +226,7 @@ func TestCaching(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(pats.oldMatches) != 3 {
+	if pats.matches.len() != 3 {
 		t.Fatal("Expected 3 cached results")
 	}
 
@@ -262,7 +238,7 @@ func TestCaching(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(pats.oldMatches) != 0 {
+	if pats.matches.len() != 0 {
 		t.Fatal("Expected cache invalidation")
 	}
 
@@ -278,7 +254,7 @@ func TestCaching(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(pats.oldMatches) != 3 {
+	if pats.matches.len() != 3 {
 		t.Fatal("Expected 3 cached results")
 	}
 }