plugin/file: load secondary zones lazily on startup (#2944)

This fixes a long standing bug:
fixes: #1609

Load secondary zones in a go-routine; this required another mutex to
protect some fields; I think those were needded anyway because a
transfer can also happen when we're running; we just didn't have a test
for that situation.

The test had to be changed to wait for the transfer to happen at this is
async now.

Signed-off-by: Miek Gieben <miek@miek.nl>

7 files changed, 21 insertions, 17 deletions

diff --git a/plugin/file/file.go b/plugin/file/file.go
index 40ebbedf7..2fe4b1644 100644
--- a/plugin/file/file.go
+++ b/plugin/file/file.go
@@ -18,8 +18,8 @@ var log = clog.NewWithPlugin("file")
 type (
 	// File is the plugin that reads zone data from disk.
 	File struct {
-		Next  plugin.Handler
-		Zones Zones
+		Next plugin.Handler
+		Zones
 	}
 
 	// Zones maps zone names to a *Zone.
diff --git a/plugin/file/lookup.go b/plugin/file/lookup.go
index 52b78114c..3a72a6163 100644
--- a/plugin/file/lookup.go
+++ b/plugin/file/lookup.go
@@ -44,7 +44,9 @@ func (z *Zone) Lookup(ctx context.Context, state request.Request, qname string)
 	// If z is a secondary zone we might not have transferred it, meaning we have
 	// all zone context setup, except the actual record. This means (for one thing) the apex
 	// is empty and we don't have a SOA record.
+	z.apexMu.RLock()
 	soa := z.Apex.SOA
+	z.apexMu.RUnlock()
 	if soa == nil {
 		return nil, nil, nil, ServerFailure
 	}
diff --git a/plugin/file/secondary.go b/plugin/file/secondary.go
index 049fe0cda..ed94daad6 100644
--- a/plugin/file/secondary.go
+++ b/plugin/file/secondary.go
@@ -51,9 +51,11 @@ Transfer:
 		return Err
 	}
 
+	z.apexMu.Lock()
 	z.Tree = z1.Tree
 	z.Apex = z1.Apex
 	*z.Expired = false
+	z.apexMu.Unlock()
 	log.Infof("Transferred: %s from %s", z.origin, tr)
 	return nil
 }
diff --git a/plugin/file/secondary_test.go b/plugin/file/secondary_test.go
index bc65c2d99..db98b4f97 100644
--- a/plugin/file/secondary_test.go
+++ b/plugin/file/secondary_test.go
@@ -80,7 +80,7 @@ func TestShouldTransfer(t *testing.T) {
 	}
 	defer s.Shutdown()
 
-	z := new(Zone)
+	z := NewZone("testzone", "test")
 	z.origin = testZone
 	z.TransferFrom = []string{addrstr}
 
diff --git a/plugin/file/zone.go b/plugin/file/zone.go
index e323350f3..b24cd91e8 100644
--- a/plugin/file/zone.go
+++ b/plugin/file/zone.go
@@ -21,7 +21,8 @@ type Zone struct {
 	origLen int
 	file    string
 	*tree.Tree
-	Apex Apex
+	Apex
+	apexMu sync.RWMutex
 
 	TransferTo   []string
 	StartupOnce  sync.Once
@@ -32,7 +33,7 @@ type Zone struct {
 	LastReloaded   time.Time
 	reloadMu       sync.RWMutex
 	reloadShutdown chan bool
-	Upstream       *upstream.Upstream // Upstream for looking up external names during the resolution process
+	Upstream       *upstream.Upstream // Upstream for looking up external names during the resolution process.
 }
 
 // Apex contains the apex records of a zone: SOA, NS and their potential signatures.
@@ -55,7 +56,6 @@ func NewZone(name, file string) *Zone {
 		LastReloaded:   time.Now(),
 	}
 	*z.Expired = false
-
 	return z
 }
 
@@ -186,11 +186,6 @@ func (z *Zone) All() []dns.RR {
 	return append([]dns.RR{z.Apex.SOA}, records...)
 }
 
-// Print prints the zone's tree to stdout.
-func (z *Zone) Print() {
-	z.Tree.Print()
-}
-
 // NameFromRight returns the labels from the right, staring with the
 // origin and then i labels extra. When we are overshooting the name
 // the returned boolean is set to true.
diff --git a/plugin/secondary/setup.go b/plugin/secondary/setup.go
index b3ca2074f..8fb9bac21 100644
--- a/plugin/secondary/setup.go
+++ b/plugin/secondary/setup.go
@@ -29,8 +29,8 @@ func setup(c *caddy.Controller) error {
 		if len(z.TransferFrom) > 0 {
 			c.OnStartup(func() error {
 				z.StartupOnce.Do(func() {
-					z.TransferIn()
 					go func() {
+						z.TransferIn()
 						z.Update()
 					}()
 				})
diff --git a/test/secondary_test.go b/test/secondary_test.go
index bb013bbaf..685e23e61 100644
--- a/test/secondary_test.go
+++ b/test/secondary_test.go
@@ -2,6 +2,7 @@ package test
 
 import (
 	"testing"
+	"time"
 
 	"github.com/coredns/coredns/plugin/test"
 
@@ -69,12 +70,16 @@ func TestSecondaryZoneTransfer(t *testing.T) {
 	m := new(dns.Msg)
 	m.SetQuestion("example.org.", dns.TypeSOA)
 
-	r, err := dns.Exchange(m, udp)
-	if err != nil {
-		t.Fatalf("Expected to receive reply, but didn't: %s", err)
+	var r *dns.Msg
+	// This is now async; we we need to wait for it to be transfered.
+	for i := 0; i < 10; i++ {
+		r, _ = dns.Exchange(m, udp)
+		if len(r.Answer) == 0 {
+			break
+		}
+		time.Sleep(100 * time.Microsecond)
 	}
-
-	if len(r.Answer) == 0 {
+	if len(r.Answer) != 0 {
 		t.Fatalf("Expected answer section")
 	}
 }