]> Cypherpunks repositories - gostls13.git/commitdiff
net: redo resolv.conf recheck implementation
authorAlex A Skinner <alex@lx.lc>
Wed, 13 May 2015 03:56:56 +0000 (23:56 -0400)
committerBrad Fitzpatrick <bradfitz@golang.org>
Fri, 15 May 2015 18:14:47 +0000 (18:14 +0000)
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
src/net/dnsclient_unix_test.go

index 5a4411f5c7a9b9542015672aefbb546e631ea1fe..fab515f5c29175683bfcdf3d9d33007a51e4f573 100644 (file)
@@ -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
                }
index 4ea24b6014af2547a4d557d890dd35b3fbb1e185..06c9ad31344ffe743fe18372fbb782aa881e7561 100644 (file)
@@ -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{}{}
 }