Merge pull request #140 from dirtbags/unmutex

Remove mutex on sort operation

src/award.go

@@ -14,6 +14,24 @@ type Award struct {
 	Points   int
 }
 
+type AwardList []*Award
+
+// Implement sort.Interface on AwardList
+func (awards AwardList) Len() int {
+  return len(awards)
+}
+
+func (awards AwardList) Less(i, j int) bool {
+  return awards[i].When.Before(awards[j].When)
+}
+
+func (awards AwardList) Swap(i, j int) {
+  tmp := awards[i]
+  awards[i] = awards[j]
+  awards[j] = tmp
+}
+
+
 func ParseAward(s string) (*Award, error) {
 	ret := Award{}
 

src/award_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"testing"
+	"sort"
 )
 
 func TestAward(t *testing.T) {
@@ -32,3 +33,23 @@ func TestAward(t *testing.T) {
 		t.Error("Not throwing error on bad points")
 	}
 }
+
+func TestAwardList(t *testing.T) {
+  a, _ := ParseAward("1536958399 1a2b3c4d counting 1")
+  b, _ := ParseAward("1536958400 1a2b3c4d counting 1")
+  c, _ := ParseAward("1536958300 1a2b3c4d counting 1")
+  list := AwardList{a, b, c}
+  
+  if sort.IsSorted(list) {
+    t.Error("Unsorted list thinks it's sorted")
+  }
+  
+  sort.Stable(list)
+  if (list[0] != c) || (list[1] != a) || (list[2] != b) {
+    t.Error("Sorting didn't")
+  }
+  
+  if ! sort.IsSorted(list) {
+    t.Error("Sorted list thinks it isn't")
+  }
+}

src/instance.go

@@ -37,7 +37,6 @@ type Instance struct {
 	nextAttempt       map[string]time.Time
 	nextAttemptMutex  *sync.RWMutex
 	mux               *http.ServeMux
-	PointsMux         *sync.RWMutex
 }
 
 func (ctx *Instance) Initialize() error {
@@ -55,7 +54,6 @@ func (ctx *Instance) Initialize() error {
 	ctx.nextAttempt = map[string]time.Time{}
 	ctx.nextAttemptMutex = new(sync.RWMutex)
 	ctx.mux = http.NewServeMux()
-	ctx.PointsMux = new(sync.RWMutex)
 
 	ctx.BindHandlers()
 	ctx.MaybeInitialize()
@@ -85,10 +83,7 @@ func (ctx *Instance) MaybeInitialize() {
 	// Remove any extant control and state files
 	os.Remove(ctx.StatePath("until"))
 	os.Remove(ctx.StatePath("disabled"))
-
-	ctx.PointsMux.Lock()
 	os.Remove(ctx.StatePath("points.log"))
-	ctx.PointsMux.Unlock()
 
 	os.RemoveAll(ctx.StatePath("points.tmp"))
 	os.RemoveAll(ctx.StatePath("points.new"))
@@ -158,18 +153,15 @@ func (ctx *Instance) TooFast(teamId string) bool {
 	return now.Before(next)
 }
 
-func (ctx *Instance) PointsLog(teamId string) []*Award {
+func (ctx *Instance) PointsLog(teamId string) AwardList {
-	var ret []*Award
+	awardlist := AwardList{}
 	
 	fn := ctx.StatePath("points.log")
 
-	ctx.PointsMux.RLock()
-	defer ctx.PointsMux.RUnlock()
-
 	f, err := os.Open(fn)
 	if err != nil {
 		log.Printf("Unable to open %s: %s", fn, err)
-		return ret
+		return awardlist
 	}
 	defer f.Close()
 
@@ -184,10 +176,10 @@ func (ctx *Instance) PointsLog(teamId string) []*Award {
 		if len(teamId) > 0 && cur.TeamId != teamId {
 			continue
 		}
-		ret = append(ret, cur)
+		awardlist = append(awardlist, cur)
 	}
 
-	return ret
+	return awardlist
 }
 
 // AwardPoints gives points to teamId in category.

src/maintenance.go

@@ -4,7 +4,6 @@ import (
 	"bufio"
 	"encoding/json"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"log"
 	"os"
@@ -205,20 +204,26 @@ func (ctx *Instance) readTeams() {
 // collectPoints gathers up files in points.new/ and appends their contents to points.log,
 // removing each points.new/ file as it goes.
 func (ctx *Instance) collectPoints() {
-	ctx.PointsMux.Lock()
+  points := ctx.PointsLog("")
-	defer ctx.PointsMux.Unlock()
 
-	logf, err := os.OpenFile(ctx.StatePath("points.log"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+  pointsFilename := ctx.StatePath("points.log")
+  pointsNewFilename := ctx.StatePath("points.log.new")
+  
+  // Yo, this is delicate.
+  // If we have to return early, we must remove this file.
+  // If the file's written and we move it successfully,
+  // we need to remove all the little points files that built it.
+	newPoints, err := os.OpenFile(pointsNewFilename, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0644)
 	if err != nil {
 		log.Printf("Can't append to points log: %s", err)
 		return
 	}
-	defer logf.Close()
 
 	files, err := ioutil.ReadDir(ctx.StatePath("points.new"))
 	if err != nil {
 		log.Printf("Error reading packages: %s", err)
 	}
+	removearino := make([]string, 0, len(files))
 	for _, f := range files {
 		filename := ctx.StatePath("points.new", f.Name())
 		s, err := ioutil.ReadFile(filename)
@@ -233,7 +238,7 @@ func (ctx *Instance) collectPoints() {
 		}
 
 		duplicate := false
-		for _, e := range ctx.PointsLog("") {
+		for _, e := range points {
 			if award.Same(e) {
 				duplicate = true
 				break
@@ -243,56 +248,33 @@ func (ctx *Instance) collectPoints() {
 		if duplicate {
 			log.Printf("Skipping duplicate points: %s", award.String())
 		} else {
-			fmt.Fprintf(logf, "%s\n", award.String())
+		  points = append(points, award)
+		}
+		removearino = append(removearino, filename)
 	}
 
-		logf.Sync()
+	sort.Stable(points)
+	for _, point := range points {
+		fmt.Fprintln(newPoints, point.String())
+	}
+	
+	newPoints.Close()
+	
+	if err := os.Rename(pointsNewFilename, pointsFilename); err != nil {
+	  log.Printf("Unable to move %s to %s: %s", pointsFilename, pointsNewFilename, err)
+	  if err := os.Remove(pointsNewFilename); err != nil {
+	    log.Printf("Also couldn't remove %s: %s", pointsNewFilename, err)
+	  }
+	  return
+	}
+
+	for _, filename := range removearino {
   	if err := os.Remove(filename); err != nil {
   		log.Printf("Unable to remove %s: %s", filename, err)
   	}
 	}
 }
 
-// Ensure that points.log is sorted chronologically
-func (ctx *Instance) sortPoints() {
-	var points []*Award
-
-	ctx.PointsMux.Lock()
-	defer ctx.PointsMux.Unlock()
-
-	logf, err := os.OpenFile(ctx.StatePath("points.log"), os.O_CREATE|os.O_RDWR, 0644)
-	if err != nil {
-		log.Printf("Can't sort points.log: %s", err)
-		return
-	}
-	defer logf.Close()
-
-	scanner := bufio.NewScanner(logf)
-	for scanner.Scan() {
-		line := scanner.Text()
-		cur, err := ParseAward(line)
-		if err != nil {
-			log.Printf("Encountered malformed award line, not sorting %s: %s", line, err)
-			return
-		}
-
-		points = append(points, cur)
-	}
-
-	// Only sort and write to file if we need to
-	if ! sort.SliceIsSorted(points, func( i, j int) bool { return points[i].When.Before(points[j].When) }) {
-
-		sort.SliceStable(points, func(i, j int) bool { return points[i].When.Before(points[j].When) })
-
-		logf.Seek(0, io.SeekStart)
-
-		for i := range points {
-			point := points[i]
-			fmt.Fprintf(logf, "%s\n", point.String())
-		}
-	}
-}
-
 func (ctx *Instance) isEnabled() bool {
 	// Skip if we've been disabled
 	if _, err := os.Stat(ctx.StatePath("disabled")); err == nil {
@@ -338,7 +320,6 @@ func (ctx *Instance) Maintenance(maintenanceInterval time.Duration) {
 			ctx.tidy()
 			ctx.readTeams()
 			ctx.collectPoints()
-			ctx.sortPoints()
 			ctx.generatePuzzleList()
 			ctx.generatePointsLog("")
 		}