diff --git a/accounts/keystore/account_cache.go b/accounts/keystore/account_cache.go index 71f698ece71a..e5d03da53cb8 100644 --- a/accounts/keystore/account_cache.go +++ b/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) } } diff --git a/accounts/keystore/account_cache_test.go b/accounts/keystore/account_cache_test.go index fe9233c046e7..0da1cc80b4be 100644 --- a/accounts/keystore/account_cache_test.go +++ b/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 { diff --git a/accounts/keystore/file_cache.go b/accounts/keystore/file_cache.go index c91b7b7b6104..754a61fd2ba9 100644 --- a/accounts/keystore/file_cache.go +++ b/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) { diff --git a/accounts/keystore/keystore_test.go b/accounts/keystore/keystore_test.go index 6fb0a7808870..b8859b23420a 100644 --- a/accounts/keystore/keystore_test.go +++ b/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) } diff --git a/accounts/keystore/watch.go b/accounts/keystore/watch.go index bbcfb99257ad..66869d0cf063 100644 --- a/accounts/keystore/watch.go +++ b/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()) } } }