From b896969d80a350ed3a019508d5d3d613fd24a528 Mon Sep 17 00:00:00 2001 From: Neale Pickett Date: Tue, 10 Mar 2020 20:50:03 -0600 Subject: [PATCH] fix for #107 that doesn't require a mutex I didn't look into why the mutex was causing some sort of deadlock condition on retrieving points. This fixes it, as long as the underlying filesystem supports atomic file renames. So don't run this with NFS. --- src/award.go | 18 +++++++++++ src/award_test.go | 21 ++++++++++++ src/instance.go | 20 ++++-------- src/maintenance.go | 81 ++++++++++++++++++---------------------------- 4 files changed, 76 insertions(+), 64 deletions(-) diff --git a/src/award.go b/src/award.go index 4a8ba75..e5dc17b 100644 --- a/src/award.go +++ b/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{} diff --git a/src/award_test.go b/src/award_test.go index 2875557..9ac9097 100644 --- a/src/award_test.go +++ b/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") + } +} diff --git a/src/instance.go b/src/instance.go index bdb9fdd..4c5c876 100644 --- a/src/instance.go +++ b/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 { - var ret []*Award - +func (ctx *Instance) PointsLog(teamId string) AwardList { + 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. diff --git a/src/maintenance.go b/src/maintenance.go index 65bb70e..829c2fc 100644 --- a/src/maintenance.go +++ b/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() - defer ctx.PointsMux.Unlock() + points := ctx.PointsLog("") - 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,53 +248,30 @@ 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) } - - logf.Sync() - 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) + removearino = append(removearino, filename) } - // 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.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 + } - 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()) - } + for _, filename := range removearino { + if err := os.Remove(filename); err != nil { + log.Printf("Unable to remove %s: %s", filename, err) + } } } @@ -338,7 +320,6 @@ func (ctx *Instance) Maintenance(maintenanceInterval time.Duration) { ctx.tidy() ctx.readTeams() ctx.collectPoints() - ctx.sortPoints() ctx.generatePuzzleList() ctx.generatePointsLog("") }