geoip2: remove select default, use defer unlock

- Remove default execution in the for-select loop with the watchFiles
  ticker (per feedback in PR #128)
- Migrate the DB-reload code into its own function to cleanly use the
  defer-unlock pattern (also per feedback in #128)

go.mod

@@ -13,6 +13,7 @@ require (
 	github.com/nxadm/tail v1.4.8
 	github.com/oschwald/geoip2-golang v1.5.0
 	github.com/pborman/uuid v1.2.1
+	github.com/pkg/errors v0.9.1 // indirect
 	github.com/prometheus/client_golang v1.11.0
 	github.com/prometheus/common v0.30.0 // indirect
 	github.com/prometheus/procfs v0.7.2 // indirect

go.sum

@@ -172,6 +172,7 @@ github.com/pborman/uuid v1.2.1 h1:+ZZIw58t/ozdjRaXh/3awHfmWRbzYxJoAdNJxe/3pvw=
 github.com/pborman/uuid v1.2.1/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k=
 github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
+github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
 github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=

targeting/geoip2/geoip2.go

@@ -14,6 +14,7 @@ import (
 	"github.com/abh/geodns/countries"
 	"github.com/abh/geodns/targeting/geo"
 	geoip2 "github.com/oschwald/geoip2-golang"
+	"github.com/pkg/errors"
 )
 
 type geoType uint8
@@ -140,48 +141,50 @@ func (g *GeoIP2) get(t geoType, db string) (*geoip2.Reader, error) {
 	return g.open(t, db)
 }
 
+// watchFiles spawns a goroutine to check for new files every minute, reloading if the modtime is newer than the original file's modtime
 func (g *GeoIP2) watchFiles() {
-	// Not worried about goroutines leaking because only one geoip2.New call is made in geodns.go (outside of testing)
+	// Not worried about goroutines leaking because only one geoip2.New call is made in main (outside of testing)
 	ticker := time.NewTicker(1 * time.Minute)
 	go func() {
 		for {
 			select {
 			case <-ticker.C:
-				g.checkForUpdate()
-			default:
-				time.Sleep(12 * time.Second) // Sleep to avoid constant looping
+				// Iterate through each file, check modtime. If new, reload file
+				d := []*geodb{&g.country, &g.city, &g.asn} // Slice of pointers is kinda gross, but want to directly reference struct values (per const type)
+				for _, v := range d {
+					fi, err := os.Stat(v.fp) // Stat is a non-blocking call we can do for (nearly) free
+					if err != nil {
+						log.Printf("unable to stat DB file at %s :: %v", v.fp, err)
+						continue
+					}
+					if fi.ModTime().UTC().Unix() > v.lastModified {
+						e := g.reloadFile(v)
+						if e != nil {
+							log.Printf("failure to update DB: %v", e)
+						}
+					}
+				}
 			}
 		}
 	}()
 }
 
-func (g *GeoIP2) checkForUpdate() {
-	// Iterate through each file, check modtime. If new, reload file
-	d := []*geodb{&g.country, &g.city, &g.asn} // Slice of pointers is kinda gross, but want to directly reference struct values (per const type)
-	for _, v := range d {
-		fi, err := os.Stat(v.fp)
-		if err != nil {
-			log.Printf("unable to stat DB file at %s :: %v", v.fp, err)
-			continue
-		}
-		if fi.ModTime().UTC().Unix() > v.lastModified {
-			g.mu.Lock()
-			e := v.db.Close()
-			if e != nil {
-				g.mu.Unlock()
-				log.Printf("unable to close DB file %s : %v", v.fp, e)
-				continue
-			}
-			n, e := geoip2.Open(v.fp)
-			if e != nil {
-				g.mu.Unlock()
-				log.Printf("unable to reopen DB file %s : %v", v.fp, e)
-				continue
-			}
-			v.db = n
-			g.mu.Unlock()
-		}
+// reloadFile wraps the DB update operation with a pointer to the geodb struct
+func (g *GeoIP2) reloadFile(v *geodb) error {
+	// Wrap this sequence of operations
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	e := v.db.Close()
+	if e != nil {
+		return errors.Wrapf(e, "unable to close DB file %s", v.fp)
+	}
+	// Directly call geoip2.Open instead of the open() function because we cannot know the related enum value for the given file.
+	n, e := geoip2.Open(v.fp)
+	if e != nil {
+		return errors.Wrapf(e, "unable to reopen DB file %s", v.fp)
 	}
+	v.db = n
+	return nil
 }
 
 // New returns a new GeoIP2 provider