From 458b0fbe109fa75f25ec76f9a3ed8210022ad1f1 Mon Sep 17 00:00:00 2001 From: Pierce Lopez Date: Fri, 23 Nov 2018 17:24:10 -0500 Subject: [PATCH 1/4] watcher: remove obsolete go1.3 build constraint --- validator_watcher_copy_test.go | 2 +- validator_watcher_test.go | 2 +- watcher.go | 2 +- watcher_unsupported.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/validator_watcher_copy_test.go b/validator_watcher_copy_test.go index 68c4cb7d3..bde4df026 100644 --- a/validator_watcher_copy_test.go +++ b/validator_watcher_copy_test.go @@ -1,4 +1,4 @@ -// +build go1.3,!plan9,!solaris,!windows +// +build !plan9,!solaris,!windows // Turns out you can't copy over an existing file on Windows. diff --git a/validator_watcher_test.go b/validator_watcher_test.go index dc16a7da3..e08d99a4d 100644 --- a/validator_watcher_test.go +++ b/validator_watcher_test.go @@ -1,4 +1,4 @@ -// +build go1.3,!plan9,!solaris +// +build !plan9,!solaris package main diff --git a/watcher.go b/watcher.go index b739d1421..48a57a229 100644 --- a/watcher.go +++ b/watcher.go @@ -1,4 +1,4 @@ -// +build go1.3,!plan9,!solaris +// +build !plan9,!solaris package main diff --git a/watcher_unsupported.go b/watcher_unsupported.go index 937f49bbc..117bdb2d4 100644 --- a/watcher_unsupported.go +++ b/watcher_unsupported.go @@ -1,4 +1,4 @@ -// +build !go1.3 plan9 solaris +// +build plan9 solaris package main From 01f391277255140429c71dc854bf8a1aace5b49e Mon Sep 17 00:00:00 2001 From: Pierce Lopez Date: Fri, 23 Nov 2018 17:24:56 -0500 Subject: [PATCH 2/4] validator: unwrap some function signature/args lines the wrapping was awkward, and we can use more than 80 columns --- validator.go | 3 +-- validator_test.go | 16 +++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/validator.go b/validator.go index 1b0492369..a54043be8 100644 --- a/validator.go +++ b/validator.go @@ -59,8 +59,7 @@ func (um *UserMap) LoadAuthenticatedEmailsFile() { atomic.StorePointer(&um.m, unsafe.Pointer(&updated)) } -func newValidatorImpl(domains []string, usersFile string, - done <-chan bool, onUpdate func()) func(string) bool { +func newValidatorImpl(domains []string, usersFile string, done <-chan bool, onUpdate func()) func(string) bool { validUsers := NewUserMap(usersFile, done, onUpdate) var allowAll bool diff --git a/validator_test.go b/validator_test.go index f91f41ce6..5763a9908 100644 --- a/validator_test.go +++ b/validator_test.go @@ -29,15 +29,13 @@ func (vt *ValidatorTest) TearDown() { os.Remove(vt.auth_email_file.Name()) } -func (vt *ValidatorTest) NewValidator(domains []string, - updated chan<- bool) func(string) bool { - return newValidatorImpl(domains, vt.auth_email_file.Name(), - vt.done, func() { - if vt.update_seen == false { - updated <- true - vt.update_seen = true - } - }) +func (vt *ValidatorTest) NewValidator(domains []string, updated chan<- bool) func(string) bool { + return newValidatorImpl(domains, vt.auth_email_file.Name(), vt.done, func() { + if vt.update_seen == false { + updated <- true + vt.update_seen = true + } + }) } // This will close vt.auth_email_file. From 260e0553468549f5b7534a28b5be873bacf0feca Mon Sep 17 00:00:00 2001 From: Pierce Lopez Date: Fri, 23 Nov 2018 19:39:21 -0500 Subject: [PATCH 3/4] tests: ValidatorTest.done chan should not be buffered ... so that ValidatorTest.TearDown() waits for the done message to be received before it continues to delete the auth_email_file. Otherwise the UserMap Watcher callback can log a Fatal error for opening the "changed" file, after the test is already done, effectively failing the next test. Also remove redundant auth_email_file.Close() but validator watcher tests "updated" chan can all have buffer of 1 --- validator_test.go | 4 ++-- validator_watcher_copy_test.go | 2 +- validator_watcher_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/validator_test.go b/validator_test.go index 5763a9908..a8b5f7dee 100644 --- a/validator_test.go +++ b/validator_test.go @@ -20,7 +20,8 @@ func NewValidatorTest(t *testing.T) *ValidatorTest { if err != nil { t.Fatal("failed to create temp file: " + err.Error()) } - vt.done = make(chan bool, 1) + t.Logf("%s using temp file %s", t.Name(), vt.auth_email_file.Name()) + vt.done = make(chan bool) return vt } @@ -40,7 +41,6 @@ func (vt *ValidatorTest) NewValidator(domains []string, updated chan<- bool) fun // This will close vt.auth_email_file. func (vt *ValidatorTest) WriteEmails(t *testing.T, emails []string) { - defer vt.auth_email_file.Close() vt.auth_email_file.WriteString(strings.Join(emails, "\n")) if err := vt.auth_email_file.Close(); err != nil { t.Fatal("failed to close temp file " + diff --git a/validator_watcher_copy_test.go b/validator_watcher_copy_test.go index bde4df026..11bbdb891 100644 --- a/validator_watcher_copy_test.go +++ b/validator_watcher_copy_test.go @@ -32,7 +32,7 @@ func TestValidatorOverwriteEmailListViaCopyingOver(t *testing.T) { vt.WriteEmails(t, []string{"xyzzy@example.com"}) domains := []string(nil) - updated := make(chan bool) + updated := make(chan bool, 1) validator := vt.NewValidator(domains, updated) if !validator("xyzzy@example.com") { diff --git a/validator_watcher_test.go b/validator_watcher_test.go index e08d99a4d..a8edbcbaa 100644 --- a/validator_watcher_test.go +++ b/validator_watcher_test.go @@ -49,7 +49,7 @@ func TestValidatorOverwriteEmailListDirectly(t *testing.T) { "plugh@example.com", }) domains := []string(nil) - updated := make(chan bool) + updated := make(chan bool, 1) validator := vt.NewValidator(domains, updated) if !validator("xyzzy@example.com") { From b42b27e3fe5bd79830aa45e4f8fae2a9bfa69085 Mon Sep 17 00:00:00 2001 From: Pierce Lopez Date: Fri, 23 Nov 2018 19:55:50 -0500 Subject: [PATCH 4/4] watcher: ignore CHMOD, limit WaitForReplacement, longer sleep fix "done" chan, only used by tests: the "break" just broke out of the select block, no the for loop could use a label, simplest fix is to just return simpler handling of CHMOD: just ignore it (if not also a rename or remove) limit WaitForReplacement to about 2 seconds (instead of waiting forever for file to re-appear) WaitForReplacement sleeps first before re-checking, for 100ms (longer) (makes travis-ci tests less flakey) --- watcher.go | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/watcher.go b/watcher.go index 48a57a229..a32e72e2a 100644 --- a/watcher.go +++ b/watcher.go @@ -11,23 +11,18 @@ import ( "github.com/fsnotify/fsnotify" ) -func WaitForReplacement(filename string, op fsnotify.Op, - watcher *fsnotify.Watcher) { - const sleep_interval = 50 * time.Millisecond +func WaitForReplacement(filename string, watcher *fsnotify.Watcher) { + for i := 0; i < 20; i++ { + time.Sleep(100 * time.Millisecond) - // Avoid a race when fsnofity.Remove is preceded by fsnotify.Chmod. - if op&fsnotify.Chmod != 0 { - time.Sleep(sleep_interval) - } - for { if _, err := os.Stat(filename); err == nil { if err := watcher.Add(filename); err == nil { log.Printf("watching resumed for %s", filename) return } } - time.Sleep(sleep_interval) } + log.Printf("failed to resume watching for %s", filename) } func WatchForUpdates(filename string, done <-chan bool, action func()) { @@ -37,22 +32,22 @@ func WatchForUpdates(filename string, done <-chan bool, action func()) { log.Fatal("failed to create watcher for ", filename, ": ", err) } go func() { - defer watcher.Close() for { select { case _ = <-done: log.Printf("Shutting down watcher for: %s", filename) - break + watcher.Close() + return case event := <-watcher.Events: // On Arch Linux, it appears Chmod events precede Remove events, // which causes a race between action() and the coming Remove event. - // If the Remove wins, the action() (which calls - // UserMap.LoadAuthenticatedEmailsFile()) crashes when the file - // can't be opened. - if event.Op&(fsnotify.Remove|fsnotify.Rename|fsnotify.Chmod) != 0 { + if event.Op == fsnotify.Chmod { + continue + } + if event.Op&(fsnotify.Remove|fsnotify.Rename) != 0 { log.Printf("watching interrupted on event: %s", event) watcher.Remove(filename) - WaitForReplacement(filename, event.Op, watcher) + WaitForReplacement(filename, watcher) } log.Printf("reloading after event: %s", event) action()