WIP accounts/keystore: don't scan all files, only file affected by event

accounts/keystore/account_cache.go

@@ -228,52 +228,92 @@ func (ac *accountCache) close() {
 	ac.mu.Unlock()
 }
 
-// scanAccounts checks if any changes have occurred on the filesystem, and
-// updates the account cache accordingly
-func (ac *accountCache) scanAccounts() error {
-	// Scan the entire folder metadata for file changes
-	creates, deletes, updates, err := ac.fileC.scan(ac.keydir)
+// readAccount is a helper-function to read an encrypted keyfile
+func readAccount(path string, buf *bufio.Reader) *accounts.Account {
+	var key struct {
+		Address string `json:"address"`
+	}
+	fd, err := os.Open(path)
+	if err != nil {
+		log.Trace("Failed to open keystore file", "path", path, "err", err)
+		return nil
+	}
+	defer fd.Close()
+	buf.Reset(fd)
+	// Parse the address.
+	key.Address = ""
+	err = json.NewDecoder(buf).Decode(&key)
+	addr := common.HexToAddress(key.Address)
+	switch {
+	case err != nil:
+		log.Debug("Failed to decode keystore key", "path", path, "err", err)
+	case (addr == common.Address{}):
+		log.Debug("Failed to decode keystore key", "path", path, "err", "missing or zero address")
+	default:
+		return &accounts.Account{Address: addr, URL: accounts.URL{Scheme: KeyStoreScheme, Path: path}}
+	}
+	return nil
+}
+
+// checkFile can be used when a file notification triggered some kind of change on a file
+// in the keystore directory. The method checks what happened (change/delete/remove/nothing) and
+// updates the keystore accordingly
+func (ac *accountCache) checkFile(path string) error {
+	start := time.Now()
+	created, deleted, updated, err := ac.fileC.checkFile(path)
+
 	if err != nil {
 		log.Debug("Failed to reload keystore contents", "err", err)
 		return err
 	}
-	if creates.Size() == 0 && deletes.Size() == 0 && updates.Size() == 0 {
+	if !created && !deleted && !updated {
 		return nil
 	}
-	// Create a helper method to scan the contents of the key files
 	var (
 		buf = new(bufio.Reader)
-		key struct {
-			Address string `json:"address"`
-		}
 	)
-	readAccount := func(path string) *accounts.Account {
-		fd, err := os.Open(path)
-		if err != nil {
-			log.Trace("Failed to open keystore file", "path", path, "err", err)
-			return nil
+	switch {
+	case created:
+		if a := readAccount(path, buf); a != nil {
+			ac.add(*a)
 		}
-		defer fd.Close()
-		buf.Reset(fd)
-		// Parse the address.
-		key.Address = ""
-		err = json.NewDecoder(buf).Decode(&key)
-		addr := common.HexToAddress(key.Address)
-		switch {
-		case err != nil:
-			log.Debug("Failed to decode keystore key", "path", path, "err", err)
-		case (addr == common.Address{}):
-			log.Debug("Failed to decode keystore key", "path", path, "err", "missing or zero address")
-		default:
-			return &accounts.Account{Address: addr, URL: accounts.URL{Scheme: KeyStoreScheme, Path: path}}
+	case deleted:
+		ac.deleteByFile(path)
+	case updated:
+		ac.deleteByFile(path)
+		if a := readAccount(path, buf); a != nil {
+			ac.add(*a)
 		}
+	}
+	select {
+	case ac.notify <- struct{}{}:
+	default:
+	}
+	end := time.Now()
+	log.Trace("Handled keystore changes", "time", end.Sub(start))
+	return nil
+}
+
+// scanAccounts checks if any changes have occurred on the filesystem, and
+// updates the account cache accordingly
+func (ac *accountCache) scanAccounts() error {
+	// Scan the entire folder metadata for file changes
+	creates, deletes, updates, err := ac.fileC.scan(ac.keydir)
+	if err != nil {
+		log.Debug("Failed to reload keystore contents", "err", err)
+		return err
+	}
+	if creates.Size() == 0 && deletes.Size() == 0 && updates.Size() == 0 {
 		return nil
 	}
 	// Process all the file diffs
 	start := time.Now()
+	var (
+		buf = new(bufio.Reader)
+	)
 
 	for _, p := range creates.List() {
-		if a := readAccount(p.(string)); a != nil {
+		if a := readAccount(p.(string), buf); a != nil {
 			ac.add(*a)
 		}
 	}
@@ -283,7 +323,7 @@ func (ac *accountCache) scanAccounts() error {
 	for _, p := range updates.List() {
 		path := p.(string)
 		ac.deleteByFile(path)
-		if a := readAccount(path); a != nil {
+		if a := readAccount(path, buf); a != nil {
 			ac.add(*a)
 		}
 	}

accounts/keystore/account_cache_test.go

@@ -51,6 +51,28 @@ var (
 	}
 )
 
+func init() {
+	rand.Seed(time.Now().UnixNano())
+}
+
+// newTempDir returns the name of new temporary folder (not created yet)
+func newTempDir() (string, error) {
+	// On OSX, there's  a problem
+	// https://stackoverflow.com/questions/45122459/docker-mounts-denied-the-paths-are-not-shared-from-os-x-and-are-not-known/45123074#45123074
+	//
+	// > /var in macOS is a symbolic link into /private.
+	//
+	// And when creating a tempdir, it returns a path into '/var/folders...'.
+	// However, if we start watching that directory, the notify-events will contain the
+	// canonical paths, and thus e.g. deleted/updated files won't match our existing files.
+	// TLDR; we need to use the canonical path, which we obtain via EvalSymlinks
+	tmpdir, err := filepath.EvalSymlinks(os.TempDir())
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(tmpdir, fmt.Sprintf("eth-keystore-watch-test-%d-%d", os.Getpid(), rand.Int())), nil
+}
+
 func TestWatchNewFile(t *testing.T) {
 	t.Parallel()
 
@@ -93,10 +115,12 @@ func TestWatchNewFile(t *testing.T) {
 
 func TestWatchNoDir(t *testing.T) {
 	t.Parallel()
-
 	// Create ks but not the directory that it watches.
-	rand.Seed(time.Now().UnixNano())
-	dir := filepath.Join(os.TempDir(), fmt.Sprintf("eth-keystore-watch-test-%d-%d", os.Getpid(), rand.Int()))
+	dir, err := newTempDir()
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	ks := NewKeyStore(dir, LightScryptN, LightScryptP)
 
 	list := ks.Accounts()
@@ -320,9 +344,11 @@ func waitForAccounts(wantAccounts []accounts.Account, ks *KeyStore) error {
 func TestUpdatedKeyfileContents(t *testing.T) {
 	t.Parallel()
 
-	// Create a temporary kesytore to test with
-	rand.Seed(time.Now().UnixNano())
-	dir := filepath.Join(os.TempDir(), fmt.Sprintf("eth-keystore-watch-test-%d-%d", os.Getpid(), rand.Int()))
+	dir, err := newTempDir()
+
+	if err != nil {
+		t.Fatal(err)
+	}
 	ks := NewKeyStore(dir, LightScryptN, LightScryptP)
 
 	list := ks.Accounts()
@@ -349,8 +375,7 @@ func TestUpdatedKeyfileContents(t *testing.T) {
 		return
 	}
 
-	// needed so that modTime of `file` is different to its current value after forceCopyFile
-	time.Sleep(1000 * time.Millisecond)
+	time.Sleep(200 * time.Millisecond)
 
 	// Now replace file contents
 	if err := forceCopyFile(file, cachetestAccounts[1].URL.Path); err != nil {
@@ -365,8 +390,7 @@ func TestUpdatedKeyfileContents(t *testing.T) {
 		return
 	}
 
-	// needed so that modTime of `file` is different to its current value after forceCopyFile
-	time.Sleep(1000 * time.Millisecond)
+	time.Sleep(200 * time.Millisecond)
 
 	// Now replace file contents again
 	if err := forceCopyFile(file, cachetestAccounts[2].URL.Path); err != nil {
@@ -381,8 +405,7 @@ func TestUpdatedKeyfileContents(t *testing.T) {
 		return
 	}
 
-	// needed so that modTime of `file` is different to its current value after ioutil.WriteFile
-	time.Sleep(1000 * time.Millisecond)
+	time.Sleep(200 * time.Millisecond)
 
 	// Now replace file contents with crap
 	if err := ioutil.WriteFile(file, []byte("foo"), 0644); err != nil {

accounts/keystore/file_cache.go

@@ -35,6 +35,29 @@ type fileCache struct {
 	mu      sync.RWMutex
 }
 
+func (fc *fileCache) checkFile(path string) (created, deleted, updated bool, err error) {
+	fc.mu.Lock()
+	defer fc.mu.Unlock()
+
+	created, deleted, updated, err = false, false, false, nil
+	previouslyKnown := fc.all.Has(path)
+	fi, err := os.Lstat(path)
+	if err != nil {
+		// A file has been deleted, but it can be a file which we
+		// were not previously watching.
+		deleted = previouslyKnown && os.IsNotExist(err)
+		return created, deleted, updated, err
+	}
+	if skipKeyFile(fi) {
+		log.Trace("Ignoring file on account scan", "path", path)
+		return created, deleted, updated, err
+	}
+
+	created = !previouslyKnown
+	updated = previouslyKnown
+	return created, deleted, updated, nil
+}
+
 // scan performs a new scan on the given directory, compares against the already
 // cached filenames, and returns file sets: creates, deletes, updates.
 func (fc *fileCache) scan(keyDir string) (set.Interface, set.Interface, set.Interface, error) {

accounts/keystore/keystore_test.go

@@ -20,6 +20,7 @@ import (
 	"io/ioutil"
 	"math/rand"
 	"os"
+	"path/filepath"
 	"runtime"
 	"sort"
 	"strings"
@@ -379,6 +380,10 @@ func tmpKeyStore(t *testing.T, encrypted bool) (string, *KeyStore) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	d, err = filepath.EvalSymlinks(d)
+	if err != nil {
+		t.Fatal(err)
+	}
 	new := NewPlaintextKeyStore
 	if encrypted {
 		new = func(kd string) *KeyStore { return NewKeyStore(kd, veryLightScryptN, veryLightScryptP) }

accounts/keystore/watch.go

@@ -19,8 +19,6 @@
 package keystore
 
 import (
-	"time"
-
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/rjeczalik/notify"
 )
@@ -77,32 +75,12 @@ func (w *watcher) loop() {
 	w.running = true
 	w.ac.mu.Unlock()
 
-	// Wait for file system events and reload.
-	// When an event occurs, the reload call is delayed a bit so that
-	// multiple events arriving quickly only cause a single reload.
-	var (
-		debounceDuration = 500 * time.Millisecond
-		rescanTriggered  = false
-		debounce         = time.NewTimer(0)
-	)
-	// Ignore initial trigger
-	if !debounce.Stop() {
-		<-debounce.C
-	}
-	defer debounce.Stop()
 	for {
 		select {
 		case <-w.quit:
 			return
-		case <-w.ev:
-			// Trigger the scan (with delay), if not already triggered
-			if !rescanTriggered {
-				debounce.Reset(debounceDuration)
-				rescanTriggered = true
-			}
-		case <-debounce.C:
-			w.ac.scanAccounts()
-			rescanTriggered = false
+		case ei := <-w.ev:
+			w.ac.checkFile(ei.Path())
 		}
 	}
 }