Merge pull request #12 from mayuresh82/race_fix

attempt to fix a race condition

controller/monitor.go

@@ -58,7 +58,7 @@ type appMon struct {
 	app       *App
 	done      chan bool
 	announced bool
-	checkOn   bool
+	runLoopOn bool
 }
 
 // MonitorMgr manages the lifecycle of registered apps
@@ -154,11 +154,12 @@ func (m *MonitorMgr) consulMon() {
 func (m *MonitorMgr) Add(app *App) {
 	// check if already running
 	m.monMu.Lock()
+	var existing *appMon
 	for _, appMon := range m.monitors {
-		if appMon.app.Equal(app) && appMon.checkOn {
-			glog.V(2).Infof("App %s already exists", app.Name)
-			m.monMu.Unlock()
-			return
+		if appMon.app.Equal(app) {
+			glog.Infof("App %s already exists", app.Name)
+			existing = appMon
+			break
 		}
 		if appMon.app.Vip.Net.String() == app.Vip.Net.String() && appMon.app.Name != app.Name {
 			glog.Errorf("Error: Vip %s is already being announced by app: %s", app.Vip.Net.String(), appMon.app.Name)
@@ -167,11 +168,19 @@ func (m *MonitorMgr) Add(app *App) {
 		}
 	}
 	m.monMu.Unlock()
-	m.Remove(app.Name)
-	appMon := &appMon{app: app, done: make(chan bool)}
-	m.monitors[app.Name] = appMon
-	go m.runLoop(appMon)
-	glog.Infof("Registered a new app: %v", app.String())
+	// if the same app already exists but its run loop is not running,
+	// then just restart the run loop
+	if existing != nil {
+		if !existing.runLoopOn {
+			go m.runLoop(existing)
+		}
+	} else {
+		// else add a new app and start its run loop
+		appMon := &appMon{app: app, done: make(chan bool)}
+		m.monitors[app.Name] = appMon
+		go m.runLoop(appMon)
+		glog.Infof("Registered a new app: %v", app.String())
+	}
 }
 
 // Remove removes an app from monitor manager, stops BGP
@@ -180,8 +189,8 @@ func (m *MonitorMgr) Remove(appName string) {
 	m.monMu.Lock()
 	defer m.monMu.Unlock()
 	if a, ok := m.monitors[appName]; ok {
-		if a.checkOn {
-			a.done <- true
+		if a.runLoopOn {
+			close(a.done)
 		}
 		if a.announced {
 			if err := m.ctrl.Withdraw(a.app.Vip); err != nil {
@@ -203,6 +212,7 @@ func (m *MonitorMgr) Remove(appName string) {
 	}
 	delete(m.monitors, appName)
 }
+
 func (m *MonitorMgr) runMonitors(app *App) bool {
 	for _, mon := range app.Monitors {
 		var check bool
@@ -250,7 +260,7 @@ func (m *MonitorMgr) checkCond(am *appMon) error {
 			}
 			am.announced = true
 			if exit, ok := m.cleanups[app.Name]; ok {
-				exit <- true
+				close(exit)
 				delete(m.cleanups, app.Name)
 			}
 		}
@@ -271,7 +281,8 @@ func (m *MonitorMgr) checkCond(am *appMon) error {
 // runLoop periodically checks if an app passes healthchecks
 // and needs VIP announcement
 func (m *MonitorMgr) runLoop(am *appMon) {
-	am.checkOn = true
+	glog.Infof("Starting run-loop for app %s", am.app.Name)
+	am.runLoopOn = true
 	if err := m.checkCond(am); err != nil {
 		glog.Errorln(err)
 	}
@@ -284,7 +295,8 @@ func (m *MonitorMgr) runLoop(am *appMon) {
 				glog.Errorln(err)
 			}
 		case <-am.done:
-			glog.V(2).Infof("Exit run-loop for app: %s", am.app.Name)
+			glog.Infof("Exit run-loop for app: %s", am.app.Name)
+			am.runLoopOn = false
 			return
 		}
 	}
@@ -297,8 +309,8 @@ func (m *MonitorMgr) CloseAll() {
 		glog.Errorf("Failed to shut-down BGP: %v", err)
 	}
 	for _, am := range m.monitors {
-		if am.checkOn {
-			am.done <- true
+		if am.runLoopOn {
+			close(am.done)
 		}
 		deleteLoopback(am.app.Vip.Net)
 		for _, nat := range am.app.Nats {
@@ -320,6 +332,7 @@ func (m *MonitorMgr) Cleanup(app string, exit chan bool) {
 		case <-t.C:
 			glog.Infof("Cleaning up app %s", app)
 			m.Remove(app)
+			return
 		case <-exit:
 			return
 		}