From bfb44f885468839259bc2ead999953cdd85654e1 Mon Sep 17 00:00:00 2001
From: Ethan Koenig <etk39@cornell.edu>
Date: Wed, 31 May 2017 04:57:17 -0400
Subject: [PATCH] Fix status table race condition (#1835)

---
 models/repo.go                   |  9 +++------
 models/repo_mirror.go            |  3 +--
 models/user.go                   |  3 +--
 modules/sync/status_pool.go      | 12 ++++++++++++
 modules/sync/status_pool_test.go |  9 +++++++++
 5 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/models/repo.go b/models/repo.go
index 9f5318e2f6..7aaeb6e0e7 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -1881,10 +1881,9 @@ func DeleteRepositoryArchives() error {
 
 // DeleteOldRepositoryArchives deletes old repository archives.
 func DeleteOldRepositoryArchives() {
-	if taskStatusTable.IsRunning(archiveCleanup) {
+	if !taskStatusTable.StartIfNotRunning(archiveCleanup) {
 		return
 	}
-	taskStatusTable.Start(archiveCleanup)
 	defer taskStatusTable.Stop(archiveCleanup)
 
 	log.Trace("Doing: ArchiveCleanup")
@@ -2025,10 +2024,9 @@ const (
 
 // GitFsck calls 'git fsck' to check repository health.
 func GitFsck() {
-	if taskStatusTable.IsRunning(gitFsck) {
+	if !taskStatusTable.StartIfNotRunning(gitFsck) {
 		return
 	}
-	taskStatusTable.Start(gitFsck)
 	defer taskStatusTable.Stop(gitFsck)
 
 	log.Trace("Doing: GitFsck")
@@ -2097,10 +2095,9 @@ func repoStatsCheck(checker *repoChecker) {
 
 // CheckRepoStats checks the repository stats
 func CheckRepoStats() {
-	if taskStatusTable.IsRunning(checkRepos) {
+	if !taskStatusTable.StartIfNotRunning(checkRepos) {
 		return
 	}
-	taskStatusTable.Start(checkRepos)
 	defer taskStatusTable.Stop(checkRepos)
 
 	log.Trace("Doing: CheckRepoStats")
diff --git a/models/repo_mirror.go b/models/repo_mirror.go
index 8a97879253..4588597743 100644
--- a/models/repo_mirror.go
+++ b/models/repo_mirror.go
@@ -202,10 +202,9 @@ func DeleteMirrorByRepoID(repoID int64) error {
 
 // MirrorUpdate checks and updates mirror repositories.
 func MirrorUpdate() {
-	if taskStatusTable.IsRunning(mirrorUpdate) {
+	if !taskStatusTable.StartIfNotRunning(mirrorUpdate) {
 		return
 	}
-	taskStatusTable.Start(mirrorUpdate)
 	defer taskStatusTable.Stop(mirrorUpdate)
 
 	log.Trace("Doing: MirrorUpdate")
diff --git a/models/user.go b/models/user.go
index 3aef510d7c..8d8afdfdb8 100644
--- a/models/user.go
+++ b/models/user.go
@@ -1334,10 +1334,9 @@ func GetWatchedRepos(userID int64, private bool) ([]*Repository, error) {
 
 // SyncExternalUsers is used to synchronize users with external authorization source
 func SyncExternalUsers() {
-	if taskStatusTable.IsRunning(syncExternalUsers) {
+	if !taskStatusTable.StartIfNotRunning(syncExternalUsers) {
 		return
 	}
-	taskStatusTable.Start(syncExternalUsers)
 	defer taskStatusTable.Stop(syncExternalUsers)
 
 	log.Trace("Doing: SyncExternalUsers")
diff --git a/modules/sync/status_pool.go b/modules/sync/status_pool.go
index 46d15aa08c..acbd93ab17 100644
--- a/modules/sync/status_pool.go
+++ b/modules/sync/status_pool.go
@@ -24,6 +24,18 @@ func NewStatusTable() *StatusTable {
 	}
 }
 
+// StartIfNotRunning sets value of given name to true if not already in pool.
+// Returns whether set value was set to true
+func (p *StatusTable) StartIfNotRunning(name string) bool {
+	p.lock.Lock()
+	_, ok := p.pool[name]
+	if !ok {
+		p.pool[name] = struct{}{}
+	}
+	p.lock.Unlock()
+	return !ok
+}
+
 // Start sets value of given name to true in the pool.
 func (p *StatusTable) Start(name string) {
 	p.lock.Lock()
diff --git a/modules/sync/status_pool_test.go b/modules/sync/status_pool_test.go
index eef3ff6f4f..b388c50db2 100644
--- a/modules/sync/status_pool_test.go
+++ b/modules/sync/status_pool_test.go
@@ -18,6 +18,15 @@ func Test_StatusTable(t *testing.T) {
 	table.Start("xyz")
 	assert.True(t, table.IsRunning("xyz"))
 
+	assert.False(t, table.StartIfNotRunning("xyz"))
+	assert.True(t, table.IsRunning("xyz"))
+
+	table.Stop("xyz")
+	assert.False(t, table.IsRunning("xyz"))
+
+	assert.True(t, table.StartIfNotRunning("xyz"))
+	assert.True(t, table.IsRunning("xyz"))
+
 	table.Stop("xyz")
 	assert.False(t, table.IsRunning("xyz"))
 }