From: Alex A Skinner Date: Wed, 13 May 2015 03:56:56 +0000 (-0400) Subject: net: redo resolv.conf recheck implementation X-Git-Tag: go1.5beta1~564 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=ef7e1085658de69b6a2e365e71a955105b3a4feb;p=gostls13.git 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 TryBot-Result: Gobot Gobot Reviewed-by: Josh Bleecher Snyder Reviewed-by: Mikio Hara Reviewed-by: Brad Fitzpatrick --- diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go index 5a4411f5c7..fab515f5c2 100644 --- a/src/net/dnsclient_unix.go +++ b/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 } diff --git a/src/net/dnsclient_unix_test.go b/src/net/dnsclient_unix_test.go index 4ea24b6014..06c9ad3134 100644 --- a/src/net/dnsclient_unix_test.go +++ b/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{}{} }