From abd14b6ddc91a2c6e27d25c45514ebb7d37dc12d Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 29 Nov 2017 10:26:18 +0100 Subject: [PATCH 1/5] accounts/keystore: don't scan all files, only file affected by event --- accounts/keystore/account_cache.go | 105 ++++++++++++++++++++--------- accounts/keystore/file_cache.go | 29 ++++++++ accounts/keystore/watch.go | 40 +++++------ 3 files changed, 124 insertions(+), 50 deletions(-) diff --git a/accounts/keystore/account_cache.go b/accounts/keystore/account_cache.go index 71f698ece7..f1707576ec 100644 --- a/accounts/keystore/account_cache.go +++ b/accounts/keystore/account_cache.go @@ -228,8 +228,77 @@ func (ac *accountCache) close() { ac.mu.Unlock() } +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 !created && !deleted && !updated { + return nil + } + var ( + buf = new(bufio.Reader) + ) + switch { + case created: + if a := readAccount(path, buf); a != nil { + ac.add(*a) + } + 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) @@ -240,40 +309,14 @@ func (ac *accountCache) scanAccounts() error { if creates.Size() == 0 && deletes.Size() == 0 && updates.Size() == 0 { 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 - } - 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 - } // 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 +326,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/file_cache.go b/accounts/keystore/file_cache.go index c91b7b7b61..96bd6368d1 100644 --- a/accounts/keystore/file_cache.go +++ b/accounts/keystore/file_cache.go @@ -35,6 +35,35 @@ 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/watch.go b/accounts/keystore/watch.go index bbcfb99257..a814f929fd 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" ) @@ -80,29 +78,33 @@ func (w *watcher) loop() { // 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) - ) + //var ( + //debounceDuration = 500 * time.Millisecond + //rescanTriggered = false + //debounce = time.NewTimer(0) + //) // Ignore initial trigger - if !debounce.Stop() { - <-debounce.C - } - defer debounce.Stop() + //if !debounce.Stop() { + // <-debounce.C + //} + //defer debounce.Stop() for { select { case <-w.quit: return - case <-w.ev: + case ei := <-w.ev: +// fmt.Printf("Event: %v\n", ei) + ei.Path() + w.ac.checkFile(ei.Path()) + // w.ac.scanAccounts() // Trigger the scan (with delay), if not already triggered - if !rescanTriggered { - debounce.Reset(debounceDuration) - rescanTriggered = true - } - case <-debounce.C: - w.ac.scanAccounts() - rescanTriggered = false + // if !rescanTriggered { + // debounce.Reset(debounceDuration) + // rescanTriggered = true + // } + // case <-debounce.C: + // w.ac.scanAccounts() + // rescanTriggered = false } } } From eaa880ad28a7393effa399287b96167d23ebbd82 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 30 Nov 2017 10:00:27 +0100 Subject: [PATCH 2/5] accounts/keystore: canonicalize path to temp directory --- accounts/keystore/account_cache.go | 2 +- accounts/keystore/account_cache_test.go | 37 +++++++++++++++++++++---- accounts/keystore/keystore_test.go | 12 +++++++- accounts/keystore/watch.go | 24 ---------------- 4 files changed, 44 insertions(+), 31 deletions(-) diff --git a/accounts/keystore/account_cache.go b/accounts/keystore/account_cache.go index f1707576ec..9881f7712c 100644 --- a/accounts/keystore/account_cache.go +++ b/accounts/keystore/account_cache.go @@ -316,7 +316,7 @@ func (ac *accountCache) scanAccounts() error { ) for _, p := range creates.List() { - if a := readAccount(p.(string),buf); a != nil { + if a := readAccount(p.(string), buf); a != nil { ac.add(*a) } } diff --git a/accounts/keystore/account_cache_test.go b/accounts/keystore/account_cache_test.go index fe9233c046..47538b10d9 100644 --- a/accounts/keystore/account_cache_test.go +++ b/accounts/keystore/account_cache_test.go @@ -51,6 +51,28 @@ var ( } ) +// 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 + + rand.Seed(time.Now().UnixNano()) + + 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() @@ -95,8 +117,11 @@ 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 +345,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() diff --git a/accounts/keystore/keystore_test.go b/accounts/keystore/keystore_test.go index 6fb0a78088..ff71bd28d4 100644 --- a/accounts/keystore/keystore_test.go +++ b/accounts/keystore/keystore_test.go @@ -26,6 +26,8 @@ import ( "testing" "time" + "path/filepath" + "github.com/ethereum/go-ethereum/accounts" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/event" @@ -375,7 +377,15 @@ func checkEvents(t *testing.T, want []walletEvent, have []walletEvent) { } func tmpKeyStore(t *testing.T, encrypted bool) (string, *KeyStore) { - d, err := ioutil.TempDir("", "eth-keystore-test") + var ( + d string + err error + ) + d, err = ioutil.TempDir("", "eth-keystore-test") + if err != nil { + t.Fatal(err) + } + d, err = filepath.EvalSymlinks(d) if err != nil { t.Fatal(err) } diff --git a/accounts/keystore/watch.go b/accounts/keystore/watch.go index a814f929fd..66869d0cf0 100644 --- a/accounts/keystore/watch.go +++ b/accounts/keystore/watch.go @@ -75,36 +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 ei := <-w.ev: -// fmt.Printf("Event: %v\n", ei) - ei.Path() w.ac.checkFile(ei.Path()) - // w.ac.scanAccounts() - // Trigger the scan (with delay), if not already triggered - // if !rescanTriggered { - // debounce.Reset(debounceDuration) - // rescanTriggered = true - // } - // case <-debounce.C: - // w.ac.scanAccounts() - // rescanTriggered = false } } } From b77771c1fb31b9349f45ba2919096e0109bfd684 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Fri, 1 Dec 2017 11:19:07 +0100 Subject: [PATCH 3/5] accounts/keystore: reverted #15498, added docs --- accounts/keystore/account_cache.go | 2 +- accounts/keystore/account_cache_test.go | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/accounts/keystore/account_cache.go b/accounts/keystore/account_cache.go index 9881f7712c..f19e6512a7 100644 --- a/accounts/keystore/account_cache.go +++ b/accounts/keystore/account_cache.go @@ -228,6 +228,7 @@ func (ac *accountCache) close() { ac.mu.Unlock() } +// readAccount is a helper-function to read an encrypted keyfile func readAccount(path string, buf *bufio.Reader) *accounts.Account { var key struct { @@ -298,7 +299,6 @@ func (ac *accountCache) checkFile(path string) error { // 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) diff --git a/accounts/keystore/account_cache_test.go b/accounts/keystore/account_cache_test.go index 47538b10d9..650ad4a74d 100644 --- a/accounts/keystore/account_cache_test.go +++ b/accounts/keystore/account_cache_test.go @@ -376,8 +376,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 { @@ -392,8 +391,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 { @@ -408,8 +406,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 { From 794564046ffe38a7b8de5ddbf6e6c768f030760d Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 13 Dec 2017 14:56:46 +0100 Subject: [PATCH 4/5] accounts/keystore: address review concerns --- accounts/keystore/account_cache.go | 3 --- accounts/keystore/account_cache_test.go | 9 ++++----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/accounts/keystore/account_cache.go b/accounts/keystore/account_cache.go index f19e6512a7..e5d03da53c 100644 --- a/accounts/keystore/account_cache.go +++ b/accounts/keystore/account_cache.go @@ -230,7 +230,6 @@ func (ac *accountCache) close() { // 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"` } @@ -254,14 +253,12 @@ func readAccount(path string, buf *bufio.Reader) *accounts.Account { 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) diff --git a/accounts/keystore/account_cache_test.go b/accounts/keystore/account_cache_test.go index 650ad4a74d..0da1cc80b4 100644 --- a/accounts/keystore/account_cache_test.go +++ b/accounts/keystore/account_cache_test.go @@ -51,9 +51,12 @@ 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 // @@ -63,9 +66,6 @@ func newTempDir() (string, error) { // 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 - - rand.Seed(time.Now().UnixNano()) - tmpdir, err := filepath.EvalSymlinks(os.TempDir()) if err != nil { return "", err @@ -115,7 +115,6 @@ func TestWatchNewFile(t *testing.T) { func TestWatchNoDir(t *testing.T) { t.Parallel() - // Create ks but not the directory that it watches. dir, err := newTempDir() if err != nil { From e798e8ff18810f4e095f518701f84fb3d9feda34 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Tue, 19 Dec 2017 14:54:16 +0100 Subject: [PATCH 5/5] accounts/keystore: fix formatting nits --- accounts/keystore/file_cache.go | 8 +------- accounts/keystore/keystore_test.go | 9 ++------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/accounts/keystore/file_cache.go b/accounts/keystore/file_cache.go index 96bd6368d1..754a61fd2b 100644 --- a/accounts/keystore/file_cache.go +++ b/accounts/keystore/file_cache.go @@ -36,23 +36,18 @@ type fileCache struct { } 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 + // 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 @@ -60,7 +55,6 @@ func (fc *fileCache) checkFile(path string) (created, deleted, updated bool, err created = !previouslyKnown updated = previouslyKnown - return created, deleted, updated, nil } diff --git a/accounts/keystore/keystore_test.go b/accounts/keystore/keystore_test.go index ff71bd28d4..b8859b2342 100644 --- a/accounts/keystore/keystore_test.go +++ b/accounts/keystore/keystore_test.go @@ -20,14 +20,13 @@ import ( "io/ioutil" "math/rand" "os" + "path/filepath" "runtime" "sort" "strings" "testing" "time" - "path/filepath" - "github.com/ethereum/go-ethereum/accounts" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/event" @@ -377,11 +376,7 @@ func checkEvents(t *testing.T, want []walletEvent, have []walletEvent) { } func tmpKeyStore(t *testing.T, encrypted bool) (string, *KeyStore) { - var ( - d string - err error - ) - d, err = ioutil.TempDir("", "eth-keystore-test") + d, err := ioutil.TempDir("", "eth-keystore-test") if err != nil { t.Fatal(err) }