Skip to content

Commit

Permalink
Provide graceful shutdown of file watcher in tests
Browse files Browse the repository at this point in the history 
This test failure from oauth2-proxy#92 inspired this change:
https://travis-ci.org/bitly/google_auth_proxy/jobs/62425336

2015/05/13 16:27:33 using authenticated emails file /tmp/test_auth_emails_952353477
2015/05/13 16:27:33 watching /tmp/test_auth_emails_952353477 for updates
2015/05/13 16:27:33 validating: is xyzzy@example.com valid? true
2015/05/13 16:27:33 watching interrupted on event: "/tmp/test_auth_emails_952353477": CHMOD
2015/05/13 16:27:33 watching resumed for /tmp/test_auth_emails_952353477
2015/05/13 16:27:33 reloading after event: "/tmp/test_auth_emails_952353477": CHMOD
2015/05/13 16:27:33 watching interrupted on event: "/tmp/test_auth_emails_952353477": REMOVE
2015/05/13 16:27:33 validating: is xyzzy@example.com valid? false
2015/05/13 16:27:33 watching resumed for /tmp/test_auth_emails_952353477
2015/05/13 16:27:33 reloading after event: "/tmp/test_auth_emails_952353477": REMOVE
2015/05/13 16:27:33 failed opening authenticated-emails-file="/tmp/test_auth_emails_952353477", open /tmp/test_auth_emails_952353477: no such file or directory

I believe that what happened was that the call to reload the file after the
second "reloading after event" lost the race when the test shut down and the
file was removed. This change introduces a `done` channel that ensures
outstanding actions complete and the watcher exits before the test removes the
file.
  • Loading branch information
Mike Bland committed May 13, 2015
1 parent 254b26d commit 6a0f119
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 17 deletions.
10 changes: 5 additions & 5 deletions validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ type UserMap struct {
m unsafe.Pointer
}

func NewUserMap(usersFile string, onUpdate func()) *UserMap {
func NewUserMap(usersFile string, done <-chan bool, onUpdate func()) *UserMap {
um := &UserMap{usersFile: usersFile}
m := make(map[string]bool)
atomic.StorePointer(&um.m, unsafe.Pointer(&m))
if usersFile != "" {
log.Printf("using authenticated emails file %s", usersFile)
started := WatchForUpdates(usersFile, func() {
started := WatchForUpdates(usersFile, done, func() {
um.LoadAuthenticatedEmailsFile()
onUpdate()
})
Expand Down Expand Up @@ -62,8 +62,8 @@ func (um *UserMap) LoadAuthenticatedEmailsFile() {
}

func newValidatorImpl(domains []string, usersFile string,
onUpdate func()) func(string) bool {
validUsers := NewUserMap(usersFile, onUpdate)
done <-chan bool, onUpdate func()) func(string) bool {
validUsers := NewUserMap(usersFile, done, onUpdate)

for i, domain := range domains {
domains[i] = fmt.Sprintf("@%s", strings.ToLower(domain))
Expand All @@ -85,5 +85,5 @@ func newValidatorImpl(domains []string, usersFile string,
}

func NewValidator(domains []string, usersFile string) func(string) bool {
return newValidatorImpl(domains, usersFile, func() {})
return newValidatorImpl(domains, usersFile, nil, func() {})
}
19 changes: 14 additions & 5 deletions validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

type ValidatorTest struct {
auth_email_file *os.File
done chan bool
}

func NewValidatorTest(t *testing.T) *ValidatorTest {
Expand All @@ -18,13 +19,21 @@ func NewValidatorTest(t *testing.T) *ValidatorTest {
if err != nil {
t.Fatal("failed to create temp file: " + err.Error())
}
vt.done = make(chan bool)
return vt
}

func (vt *ValidatorTest) TearDown() {
vt.done <- true
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() { updated <- true })
}

// This will close vt.auth_email_file.
func (vt *ValidatorTest) WriteEmails(t *testing.T, emails []string) {
defer vt.auth_email_file.Close()
Expand All @@ -41,7 +50,7 @@ func TestValidatorEmpty(t *testing.T) {

vt.WriteEmails(t, []string(nil))
domains := []string(nil)
validator := NewValidator(domains, vt.auth_email_file.Name())
validator := vt.NewValidator(domains, nil)

if validator("foo.bar@example.com") {
t.Error("nothing should validate when the email and " +
Expand All @@ -55,7 +64,7 @@ func TestValidatorSingleEmail(t *testing.T) {

vt.WriteEmails(t, []string{"foo.bar@example.com"})
domains := []string(nil)
validator := NewValidator(domains, vt.auth_email_file.Name())
validator := vt.NewValidator(domains, nil)

if !validator("foo.bar@example.com") {
t.Error("email should validate")
Expand All @@ -72,7 +81,7 @@ func TestValidatorSingleDomain(t *testing.T) {

vt.WriteEmails(t, []string(nil))
domains := []string{"example.com"}
validator := NewValidator(domains, vt.auth_email_file.Name())
validator := vt.NewValidator(domains, nil)

if !validator("foo.bar@example.com") {
t.Error("email should validate")
Expand All @@ -91,7 +100,7 @@ func TestValidatorMultipleEmailsMultipleDomains(t *testing.T) {
"plugh@example.com",
})
domains := []string{"example0.com", "example1.com"}
validator := NewValidator(domains, vt.auth_email_file.Name())
validator := vt.NewValidator(domains, nil)

if !validator("foo.bar@example0.com") {
t.Error("email from first domain should validate")
Expand All @@ -117,7 +126,7 @@ func TestValidatorComparisonsAreCaseInsensitive(t *testing.T) {

vt.WriteEmails(t, []string{"Foo.Bar@Example.Com"})
domains := []string{"Frobozz.Com"}
validator := NewValidator(domains, vt.auth_email_file.Name())
validator := vt.NewValidator(domains, nil)

if !validator("foo.bar@example.com") {
t.Error("loaded email addresses are not lower-cased")
Expand Down
3 changes: 1 addition & 2 deletions validator_watcher_copy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ func TestValidatorOverwriteEmailListViaCopyingOver(t *testing.T) {
vt.WriteEmails(t, []string{"xyzzy@example.com"})
domains := []string(nil)
updated := make(chan bool)
validator := newValidatorImpl(domains, vt.auth_email_file.Name(),
func() { updated <- true })
validator := vt.NewValidator(domains, updated)

if !validator("xyzzy@example.com") {
t.Error("email in list should validate")
Expand Down
6 changes: 2 additions & 4 deletions validator_watcher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ func TestValidatorOverwriteEmailListDirectly(t *testing.T) {
})
domains := []string(nil)
updated := make(chan bool)
validator := newValidatorImpl(domains, vt.auth_email_file.Name(),
func() { updated <- true })
validator := vt.NewValidator(domains, updated)

if !validator("xyzzy@example.com") {
t.Error("first email in list should validate")
Expand Down Expand Up @@ -89,8 +88,7 @@ func TestValidatorOverwriteEmailListViaRenameAndReplace(t *testing.T) {
vt.WriteEmails(t, []string{"xyzzy@example.com"})
domains := []string(nil)
updated := make(chan bool)
validator := newValidatorImpl(domains, vt.auth_email_file.Name(),
func() { updated <- true })
validator := vt.NewValidator(domains, updated)

if !validator("xyzzy@example.com") {
t.Error("email in list should validate")
Expand Down
6 changes: 5 additions & 1 deletion watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func WaitForReplacement(event fsnotify.Event, watcher *fsnotify.Watcher) {
}
}

func WatchForUpdates(filename string, action func()) bool {
func WatchForUpdates(filename string, done <-chan bool, action func()) bool {
filename = filepath.Clean(filename)
watcher, err := fsnotify.NewWatcher()
if err != nil {
Expand All @@ -40,6 +40,10 @@ func WatchForUpdates(filename string, action func()) bool {
defer watcher.Close()
for {
select {
case _ = <-done:
log.Printf("Shutting down watcher for: %s",
filename)
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.
Expand Down

0 comments on commit 6a0f119

Please sign in to comment.