net: redo resolv.conf recheck implementation

The previous implementation spawned an extra goroutine to handle
rechecking resolv.conf for changes.

This change eliminates the extra goroutine, and has rechecking
done as part of a lookup.  A side effect of this change is that the
first lookup after a resolv.conf change will now succeed, whereas
previously it would have failed.  It also fixes rechecking logic to
ignore resolv.conf parsing errors as it should.

Fixes #8652
Fixes #10576
Fixes #10649
Fixes #10650
Fixes #10845

Change-Id: I502b587c445fa8eca5207ca4f2c8ec8c339fec7f
Reviewed-on: https://go-review.googlesource.com/9991
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

src/net/dnsclient_unix.go

@@ -214,95 +214,106 @@ func convertRR_AAAA(records []dnsRR) []IP {
 	return addrs
 }
 
+// cfg is used for the storage and reparsing of /etc/resolv.conf
 var cfg struct {
-	ch        chan struct{}
+	// ch is used as a semaphore that only allows one lookup at a time to
+	// recheck resolv.conf.  It acts as guard for lastChecked and modTime.
+	ch          chan struct{}
+	lastChecked time.Time // last time resolv.conf was checked
+	modTime     time.Time // time of resolv.conf modification
+
 	mu        sync.RWMutex // protects dnsConfig
-	dnsConfig *dnsConfig
+	dnsConfig *dnsConfig   // parsed resolv.conf structure used in lookups
 }
 
 var onceLoadConfig sync.Once
 
-// Assume dns config file is /etc/resolv.conf here
-func loadDefaultConfig() {
-	loadConfig("/etc/resolv.conf", 5*time.Second, nil)
-}
+func initCfg() {
+	// Set dnsConfig, modTime, and lastChecked so we don't parse
+	// resolv.conf twice the first time.
+	cfg.dnsConfig = systemConf().resolv
+	if cfg.dnsConfig == nil {
+		cfg.dnsConfig = dnsReadConfig("/etc/resolv.conf")
+	}
 
-func loadConfig(resolvConfPath string, reloadTime time.Duration, quit <-chan chan struct{}) {
-	var mtime time.Time
-	cfg.ch = make(chan struct{}, 1)
-	if fi, err := os.Stat(resolvConfPath); err == nil {
-		mtime = fi.ModTime()
+	if fi, err := os.Stat("/etc/resolv.conf"); err == nil {
+		cfg.modTime = fi.ModTime()
 	}
+	cfg.lastChecked = time.Now()
 
-	cfg.dnsConfig = dnsReadConfig(resolvConfPath)
+	// Prepare ch so that only one loadConfig may run at once
+	cfg.ch = make(chan struct{}, 1)
+	cfg.ch <- struct{}{}
+}
 
-	go func() {
-		for {
-			time.Sleep(reloadTime)
-			select {
-			case qresp := <-quit:
-				qresp <- struct{}{}
-				return
-			case <-cfg.ch:
-			}
+func loadConfig(resolvConfPath string) {
+	onceLoadConfig.Do(initCfg)
 
-			// In case of error, we keep the previous config
-			fi, err := os.Stat(resolvConfPath)
-			if err != nil {
-				continue
-			}
-			// If the resolv.conf mtime didn't change, do not reload
-			m := fi.ModTime()
-			if m.Equal(mtime) {
-				continue
-			}
-			mtime = m
-			// In case of error, we keep the previous config
-			if ncfg := dnsReadConfig(resolvConfPath); ncfg.err == nil {
-				cfg.mu.Lock()
-				cfg.dnsConfig = ncfg
-				cfg.mu.Unlock()
-			}
+	// ensure only one loadConfig at a time checks /etc/resolv.conf
+	select {
+	case <-cfg.ch:
+		defer func() { cfg.ch <- struct{}{} }()
+	default:
+		return
+	}
+
+	now := time.Now()
+	if cfg.lastChecked.After(now.Add(-5 * time.Second)) {
+		return
+	}
+	cfg.lastChecked = now
+
+	if fi, err := os.Stat(resolvConfPath); err == nil {
+		if fi.ModTime().Equal(cfg.modTime) {
+			return
 		}
-	}()
+		cfg.modTime = fi.ModTime()
+	} else {
+		// If modTime wasn't set prior, assume nothing has changed.
+		if cfg.modTime.IsZero() {
+			return
+		}
+		cfg.modTime = time.Time{}
+	}
+
+	ncfg := dnsReadConfig(resolvConfPath)
+	cfg.mu.Lock()
+	cfg.dnsConfig = ncfg
+	cfg.mu.Unlock()
 }
 
 func lookup(name string, qtype uint16) (cname string, rrs []dnsRR, err error) {
 	if !isDomainName(name) {
 		return name, nil, &DNSError{Err: "invalid domain name", Name: name}
 	}
-	onceLoadConfig.Do(loadDefaultConfig)
-
-	select {
-	case cfg.ch <- struct{}{}:
-	default:
-	}
 
+	loadConfig("/etc/resolv.conf")
 	cfg.mu.RLock()
-	defer cfg.mu.RUnlock()
+	resolv := cfg.dnsConfig
+	cfg.mu.RUnlock()
 
 	// If name is rooted (trailing dot) or has enough dots,
 	// try it by itself first.
 	rooted := len(name) > 0 && name[len(name)-1] == '.'
-	if rooted || count(name, '.') >= cfg.dnsConfig.ndots {
+	if rooted || count(name, '.') >= resolv.ndots {
 		rname := name
 		if !rooted {
 			rname += "."
 		}
 		// Can try as ordinary name.
-		cname, rrs, err = tryOneName(cfg.dnsConfig, rname, qtype)
+		cname, rrs, err = tryOneName(resolv, rname, qtype)
 		if rooted || err == nil {
 			return
 		}
 	}
 
 	// Otherwise, try suffixes.
-	for i := 0; i < len(cfg.dnsConfig.search); i++ {
-		rname := name + "." + cfg.dnsConfig.search[i]
+	for _, suffix := range resolv.search {
+		rname := name + "." + suffix
 		if rname[len(rname)-1] != '.' {
 			rname += "."
 		}
-		cname, rrs, err = tryOneName(cfg.dnsConfig, rname, qtype)
+		cname, rrs, err = tryOneName(resolv, rname, qtype)
 		if err == nil {
 			return
 		}
@@ -310,8 +321,8 @@ func lookup(name string, qtype uint16) (cname string, rrs []dnsRR, err error) {
 
 	// Last ditch effort: try unsuffixed only if we haven't already,
 	// that is, name is not rooted and has less than ndots dots.
-	if count(name, '.') < cfg.dnsConfig.ndots {
-		cname, rrs, err = tryOneName(cfg.dnsConfig, name+".", qtype)
+	if count(name, '.') < resolv.ndots {
+		cname, rrs, err = tryOneName(resolv, name+".", qtype)
 		if err == nil {
 			return
 		}

src/net/dnsclient_unix_test.go

@@ -94,10 +94,8 @@ func TestSpecialDomainName(t *testing.T) {
 
 type resolvConfTest struct {
 	*testing.T
-	dir     string
-	path    string
-	started bool
-	quitc   chan chan struct{}
+	dir  string
+	path string
 }
 
 func newResolvConfTest(t *testing.T) *resolvConfTest {
@@ -106,24 +104,15 @@ func newResolvConfTest(t *testing.T) *resolvConfTest {
 		t.Fatal(err)
 	}
 
-	// Disable the default loadConfig
-	onceLoadConfig.Do(func() {})
-
 	r := &resolvConfTest{
-		T:     t,
-		dir:   dir,
-		path:  path.Join(dir, "resolv.conf"),
-		quitc: make(chan chan struct{}),
+		T:    t,
+		dir:  dir,
+		path: path.Join(dir, "resolv.conf"),
 	}
 
 	return r
 }
 
-func (r *resolvConfTest) Start() {
-	loadConfig(r.path, 100*time.Millisecond, r.quitc)
-	r.started = true
-}
-
 func (r *resolvConfTest) SetConf(s string) {
 	// Make sure the file mtime will be different once we're done here,
 	// even on systems with coarse (1s) mtime resolution.
@@ -138,12 +127,8 @@ func (r *resolvConfTest) SetConf(s string) {
 		r.Fatalf("failed to write temp file: %v", err)
 	}
 	f.Close()
-
-	if r.started {
-		cfg.ch <- struct{}{} // fill buffer
-		cfg.ch <- struct{}{} // wait for reload to begin
-		cfg.ch <- struct{}{} // wait for reload to complete
-	}
+	cfg.lastChecked = time.Time{}
+	loadConfig(r.path)
 }
 
 func (r *resolvConfTest) WantServers(want []string) {
@@ -155,9 +140,6 @@ func (r *resolvConfTest) WantServers(want []string) {
 }
 
 func (r *resolvConfTest) Close() {
-	resp := make(chan struct{})
-	r.quitc <- resp
-	<-resp
 	if err := os.RemoveAll(r.dir); err != nil {
 		r.Logf("failed to remove temp dir %s: %v", r.dir, err)
 	}
@@ -171,7 +153,6 @@ func TestReloadResolvConfFail(t *testing.T) {
 	r := newResolvConfTest(t)
 	defer r.Close()
 
-	r.Start()
 	r.SetConf("nameserver 8.8.8.8")
 
 	if _, err := goLookupIP("golang.org"); err != nil {
@@ -200,7 +181,6 @@ func TestReloadResolvConfChange(t *testing.T) {
 	r := newResolvConfTest(t)
 	defer r.Close()
 
-	r.Start()
 	r.SetConf("nameserver 8.8.8.8")
 
 	if _, err := goLookupIP("golang.org"); err != nil {
@@ -245,14 +225,14 @@ func BenchmarkGoLookupIPNoSuchHost(b *testing.B) {
 func BenchmarkGoLookupIPWithBrokenNameServer(b *testing.B) {
 	testHookUninstaller.Do(uninstallTestHooks)
 
-	onceLoadConfig.Do(loadDefaultConfig)
-
 	// This looks ugly but it's safe as long as benchmarks are run
 	// sequentially in package testing.
+	<-cfg.ch // keep config from being reloaded upon lookup
 	orig := cfg.dnsConfig
 	cfg.dnsConfig.servers = append([]string{"203.0.113.254"}, cfg.dnsConfig.servers...) // use TEST-NET-3 block, see RFC 5737
 	for i := 0; i < b.N; i++ {
 		goLookupIP("www.example.com")
 	}
 	cfg.dnsConfig = orig
+	cfg.ch <- struct{}{}
 }