|
|
Descriptionnet: detect changes to /etc/resolv.conf.
Implement the changes as suggested by rsc.
Fixes issue 6670.
Patch Set 1 #Patch Set 2 : diff -r ddfee9dfca20 https://code.google.com/p/go #Patch Set 3 : diff -r ddfee9dfca20 https://code.google.com/p/go #Patch Set 4 : diff -r ddfee9dfca20 https://code.google.com/p/go #
Total comments: 8
Patch Set 5 : diff -r 88b3b2fa4dde https://code.google.com/p/go #Patch Set 6 : diff -r 88b3b2fa4dde https://code.google.com/p/go #Patch Set 7 : diff -r 88b3b2fa4dde https://code.google.com/p/go #Patch Set 8 : diff -r 88b3b2fa4dde https://code.google.com/p/go #Patch Set 9 : diff -r 88b3b2fa4dde https://code.google.com/p/go #
Total comments: 3
Patch Set 10 : diff -r 88b3b2fa4dde https://code.google.com/p/go #Patch Set 11 : diff -r 88b3b2fa4dde https://code.google.com/p/go #Patch Set 12 : diff -r 88b3b2fa4dde https://code.google.com/p/go #
Total comments: 3
Patch Set 13 : diff -r b3405f9c2e32 https://code.google.com/p/go #Patch Set 14 : diff -r b3405f9c2e32 https://code.google.com/p/go #Patch Set 15 : diff -r b3405f9c2e32 https://code.google.com/p/go #Patch Set 16 : diff -r 4c91e8fa0ebf https://code.google.com/p/go #Patch Set 17 : diff -r 4c91e8fa0ebf https://code.google.com/p/go #Patch Set 18 : diff -r 1bf549d894c9 https://code.google.com/p/go #Patch Set 19 : diff -r 1bf549d894c9 https://code.google.com/p/go #Patch Set 20 : diff -r 1bf549d894c9 https://code.google.com/p/go #Patch Set 21 : diff -r f8b50ad4cac4 https://code.google.com/p/go #Patch Set 22 : diff -r 1d8991d40dd3 https://code.google.com/p/go #Patch Set 23 : diff -r 1d8991d40dd3 https://code.google.com/p/go #
Total comments: 2
Patch Set 24 : diff -r 9fd0fe734a42 https://code.google.com/p/go #
MessagesTotal messages: 48
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com, rsc@golang.org), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:168: cfg.ch = make(chan struct{}) This should be a buffered channel. Otherwise there is a race for whether the wakeup signal is seen. A buffer size of 1 is fine. https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:175: if ncfg, err := dnsReadConfig("/etc/resolv.conf"); err == nil { We just want to stat the file here. We only want to reread it if the mtime changes.
Sign in to reply to this message.
Thanks for working on this! I like how simple this is. It'd be nice to have a few tests, e.g.: * Go from bad config to good to bad again * Go from good config to bad to different good Is it worth skipping the re-parsing in the common case (no changes) by doing a stat first and looking at the mtime? https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:11: // Check periodically whether /etc/resolv.conf has changed. Remove this TODO. https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:160: mu sync.RWMutex Move mu after ch, since it only protects dnsConfig. https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:178: cfg.dnsConfig = ncfg I think you also need to set dnserr to nil here. Otherwise, if the first dnsReadConfig fails, dnserr will forever be non-nil, and all lookups will fail forever. And given that, dnserr probably should be part of cfg. https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:265: onceLoadConfig.Do(loadConfig) Do you want to send to cfg.ch here as well? (Why only in lookup?) https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:303: onceLoadConfig.Do(loadConfig) Do you want to send to cfg.ch here as well? (Why only in lookup?) https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... src/pkg/net/dnsclient_unix.go:345: func goLookupCNAME(name string) (cname string, err error) { Why is it now ok to ignore non-nil dnserr here?
Sign in to reply to this message.
On 2014/04/03 23:47:14, iant wrote: > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:168: cfg.ch = make(chan struct{}) > This should be a buffered channel. Otherwise there is a race for whether the > wakeup signal is seen. A buffer size of 1 is fine. > Having a buffered chan would cause a potential unnecessary reload as the send wouldn't be blocking. It is time based, arbitrary set to 5sec, if the signal is not seen at 5s000000001 but at 5s000000002 or even 6s, I think it is acceptable. If you have a better solution, I'll take it. > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:175: if ncfg, err := > dnsReadConfig("/etc/resolv.conf"); err == nil { > We just want to stat the file here. We only want to reread it if the mtime > changes. Will do
Sign in to reply to this message.
On 2014/04/03 23:55:54, creack wrote: > On 2014/04/03 23:47:14, iant wrote: > > > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go > > File src/pkg/net/dnsclient_unix.go (right): > > > > > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > > src/pkg/net/dnsclient_unix.go:168: cfg.ch = make(chan struct{}) > > This should be a buffered channel. Otherwise there is a race for whether the > > wakeup signal is seen. A buffer size of 1 is fine. > > > > Having a buffered chan would cause a potential unnecessary reload as the send > wouldn't be blocking. > It is time based, arbitrary set to 5sec, if the signal is not seen at > 5s000000001 but at 5s000000002 or even 6s, > I think it is acceptable. > If you have a better solution, I'll take it. While you're technically right, I'm with Ian on this. With a buffered channel, you might end up doing one reload too many after all network activity dies -- not so bad. On the upside, though, the reload happens greedily; with a non-buffered channel, the lookup that triggers the dns reload is unlikely to get to benefit from the reload.
Sign in to reply to this message.
On 2014/04/03 23:54:14, josharian wrote: > Thanks for working on this! I like how simple this is. > > It'd be nice to have a few tests, e.g.: > > * Go from bad config to good to bad again > * Go from good config to bad to different good > will do > Is it worth skipping the re-parsing in the common case (no changes) by doing a > stat first and looking at the mtime? > I think so, the file does not change very often and stat is cheaper faster than a whole read/parse > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:11: // Check periodically whether /etc/resolv.conf > has changed. > Remove this TODO. > missed that one > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:160: mu sync.RWMutex > Move mu after ch, since it only protects dnsConfig. > ok > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:178: cfg.dnsConfig = ncfg > I think you also need to set dnserr to nil here. Otherwise, if the first > dnsReadConfig fails, dnserr will forever be non-nil, and all lookups will fail > forever. And given that, dnserr probably should be part of cfg. > Indeed. Will do. > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:265: onceLoadConfig.Do(loadConfig) > Do you want to send to cfg.ch here as well? (Why only in lookup?) > As every function relies on lookup, and only lookup uses cfg, I send the signal only there. > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:303: onceLoadConfig.Do(loadConfig) > Do you want to send to cfg.ch here as well? (Why only in lookup?) > As every function relies on lookup, and only lookup uses cfg, I send the signal only there. > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > src/pkg/net/dnsclient_unix.go:345: func goLookupCNAME(name string) (cname > string, err error) { > Why is it now ok to ignore non-nil dnserr here? Because it goes directly to lookup with does the check
Sign in to reply to this message.
> https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > > src/pkg/net/dnsclient_unix.go:265: onceLoadConfig.Do(loadConfig) > > Do you want to send to cfg.ch here as well? (Why only in lookup?) > > > > As every function relies on lookup, and only lookup uses cfg, I send the signal > only there. Ahh, I missed that. Thanks. > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix... > > src/pkg/net/dnsclient_unix.go:345: func goLookupCNAME(name string) (cname > > string, err error) { > > Why is it now ok to ignore non-nil dnserr here? > > Because it goes directly to lookup with does the check Indeed...I wonder why it was ever different.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
please try to run "CGO_ENABLED=0 go test net -v -cpu=2,4,8" and see what happens with your cl. also, will keep Alex in the loop. https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:159: var cfg struct { do we really need this indirection? why not change dnsConfig.
Sign in to reply to this message.
On 2014/04/05 10:11:54, mikio wrote: > please try to run "CGO_ENABLED=0 go test net -v -cpu=2,4,8" and see what happens > with your cl. also, will keep Alex in the loop. > CGO_ENABLED=0 go test net -v -cpu=2,4,8 ... ... PASS ok net 163.139s > https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... > src/pkg/net/dnsclient_unix.go:159: var cfg struct { > do we really need this indirection? why not change dnsConfig. It is not directly linked to dnsConfig, it reloads it, so I think it is best to have a struct here.
Sign in to reply to this message.
Are we considering for 1.3, or going to wait to merge into other logic fixes pending review(for 1.4, I guess)? https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:284: onceLoadConfig.Do(loadDefaultConfig) Since this calls goLookupIP, which calls lookup, the err should come back from lookup in the end(after minimal allocs, sure). Should we remove this block as you did for CNAME. https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:322: onceLoadConfig.Do(loadDefaultConfig) calls lookup, which does this. See above comment. Can we clean up?
Sign in to reply to this message.
On 2014/04/05 17:08:03, axaxs wrote: > Are we considering for 1.3, or going to wait to merge into other logic fixes > pending review(for 1.4, I guess)? > > https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... > src/pkg/net/dnsclient_unix.go:284: onceLoadConfig.Do(loadDefaultConfig) > Since this calls goLookupIP, which calls lookup, the err should come back from > lookup in the end(after minimal allocs, sure). Should we remove this block as > you did for CNAME. > > https://codereview.appspot.com/83690045/diff/160001/src/pkg/net/dnsclient_uni... > src/pkg/net/dnsclient_unix.go:322: onceLoadConfig.Do(loadDefaultConfig) > calls lookup, which does this. See above comment. Can we clean up? I don't know about 1.3/1.4, from what I understood, we past the feature freeze for 1.3 :(. For the error check, I though about this, but I think we should keep them to speedup the process in case of error.
Sign in to reply to this message.
> CGO_ENABLED=0 go test net -v -cpu=2,4,8 > ... > PASS > ok net 163.139s hm, i have a bit different result on linux. # CGO_ENABLED=0 go test net -cpu=1,2,4 (snip) --- FAIL: TestReloadResolvConfChange (103.32 seconds) dnsclient_unix_test.go:82: goLookupIP should failed: %v lookup golang.org on 192.168.1.1:53: no answer from server --- FAIL: TestGoogleSRV (60.17 seconds) lookup_test.go:24: failed: lookup _xmpp-server._tcp.google.com on 192.168.1.1:53: no answer from server lookup_test.go:27: no results FAIL perhaps you need to run go tool vet. also looks like your cl has a race condition. WARNING: DATA RACE Write by goroutine 350: net.loadConfig() /home/mikioh/go/src/pkg/net/dnsclient_unix.go:177 +0x217 net.TestReloadResolvConfFail() /home/mikioh/go/src/pkg/net/dnsclient_unix_test.go:39 +0x125 (snip) Found 5 data race(s) FAIL net 135.153s > > src/pkg/net/dnsclient_unix.go:159: var cfg struct { > > do we really need this indirection? why not change dnsConfig. > > It is not directly linked to dnsConfig, it reloads it, so I think it is best to > have a struct here. seems the new indirection introduces a few race conditions.
Sign in to reply to this message.
From a quick glance, looks like one of the inner lookups uses "/etc/resolv.conf" instead resolvConfPath in one place, which could contribute to the test result mismatches. Will look closer on Monday. On Saturday, April 5, 2014, <mikioh.mikioh@gmail.com> wrote: > CGO_ENABLED=0 go test net -v -cpu=2,4,8 >> ... >> PASS >> ok net 163.139s >> > > hm, i have a bit different result on linux. > > # CGO_ENABLED=0 go test net -cpu=1,2,4 > (snip) > --- FAIL: TestReloadResolvConfChange (103.32 seconds) > dnsclient_unix_test.go:82: goLookupIP should failed: %v lookup > golang.org on 192.168.1.1:53: no answer from server > --- FAIL: TestGoogleSRV (60.17 seconds) > lookup_test.go:24: failed: lookup _xmpp-server._tcp.google.com on > 192.168.1.1:53: no answer from server > lookup_test.go:27: no results > FAIL > > perhaps you need to run go tool vet. also looks like your cl has a race > condition. > > WARNING: DATA RACE > Write by goroutine 350: > net.loadConfig() > /home/mikioh/go/src/pkg/net/dnsclient_unix.go:177 +0x217 > net.TestReloadResolvConfFail() > /home/mikioh/go/src/pkg/net/dnsclient_unix_test.go:39 +0x125 > (snip) > Found 5 data race(s) > FAIL net 135.153s > > > src/pkg/net/dnsclient_unix.go:159: var cfg struct { >> > do we really need this indirection? why not change dnsConfig. >> > > It is not directly linked to dnsConfig, it reloads it, so I think it >> > is best to > >> have a struct here. >> > > seems the new indirection introduces a few race conditions. > > https://codereview.appspot.com/83690045/ >
Sign in to reply to this message.
On 2014/04/05 17:45:13, josharian wrote: > From a quick glance, looks like one of the inner lookups uses > "/etc/resolv.conf" instead resolvConfPath in one place, which could > contribute to the test result mismatches. > > Will look closer on Monday. > > > On Saturday, April 5, 2014, <mailto:mikioh.mikioh@gmail.com> wrote: > > > CGO_ENABLED=0 go test net -v -cpu=2,4,8 > >> ... > >> PASS > >> ok net 163.139s > >> > > > > hm, i have a bit different result on linux. > > > > # CGO_ENABLED=0 go test net -cpu=1,2,4 > > (snip) > > --- FAIL: TestReloadResolvConfChange (103.32 seconds) > > dnsclient_unix_test.go:82: goLookupIP should failed: %v lookup > > http://golang.org on 192.168.1.1:53: no answer from server > > --- FAIL: TestGoogleSRV (60.17 seconds) > > lookup_test.go:24: failed: lookup http://_xmpp-server._tcp.google.com on > > 192.168.1.1:53: no answer from server > > lookup_test.go:27: no results > > FAIL > > > > perhaps you need to run go tool vet. also looks like your cl has a race > > condition. > > > > WARNING: DATA RACE > > Write by goroutine 350: > > net.loadConfig() > > /home/mikioh/go/src/pkg/net/dnsclient_unix.go:177 +0x217 > > net.TestReloadResolvConfFail() > > /home/mikioh/go/src/pkg/net/dnsclient_unix_test.go:39 +0x125 > > (snip) > > Found 5 data race(s) > > FAIL net 135.153s > > > > > src/pkg/net/dnsclient_unix.go:159: var cfg struct { > >> > do we really need this indirection? why not change dnsConfig. > >> > > > > It is not directly linked to dnsConfig, it reloads it, so I think it > >> > > is best to > > > >> have a struct here. > >> > > > > seems the new indirection introduces a few race conditions. > > > > https://codereview.appspot.com/83690045/ > > Indeed, I missed one, thx.
Sign in to reply to this message.
On 2014/04/05 17:35:35, mikio wrote: > > CGO_ENABLED=0 go test net -v -cpu=2,4,8 > > ... > > PASS > > ok net 163.139s > > hm, i have a bit different result on linux. > > # CGO_ENABLED=0 go test net -cpu=1,2,4 > (snip) > --- FAIL: TestReloadResolvConfChange (103.32 seconds) > dnsclient_unix_test.go:82: goLookupIP should failed: %v lookup http://golang.org on > 192.168.1.1:53: no answer from server > --- FAIL: TestGoogleSRV (60.17 seconds) > lookup_test.go:24: failed: lookup http://_xmpp-server._tcp.google.com on > 192.168.1.1:53: no answer from server > lookup_test.go:27: no results > FAIL > I fixed the govet issues, fixed a missing resolvConfPath and updated the test. Should be fixed. > perhaps you need to run go tool vet. also looks like your cl has a race > condition. > > WARNING: DATA RACE > Write by goroutine 350: > net.loadConfig() > /home/mikioh/go/src/pkg/net/dnsclient_unix.go:177 +0x217 > net.TestReloadResolvConfFail() > /home/mikioh/go/src/pkg/net/dnsclient_unix_test.go:39 +0x125 > (snip) > Found 5 data race(s) > FAIL net 135.153s > I do not manage to reproduce this race with -race. Could you send me the whole output? The line 177 is protected by sync.Once, I'd be interested to see who is reading it as the same time. > > > src/pkg/net/dnsclient_unix.go:159: var cfg struct { > > > do we really need this indirection? why not change dnsConfig. > > > > It is not directly linked to dnsConfig, it reloads it, so I think it is best > to > > have a struct here. > > seems the new indirection introduces a few race conditions.
Sign in to reply to this message.
On 2014/04/05 18:01:53, creack wrote: > On 2014/04/05 17:35:35, mikio wrote: > > > CGO_ENABLED=0 go test net -v -cpu=2,4,8 > > > ... > > > PASS > > > ok net 163.139s > > > > hm, i have a bit different result on linux. > > > > # CGO_ENABLED=0 go test net -cpu=1,2,4 > > (snip) > > --- FAIL: TestReloadResolvConfChange (103.32 seconds) > > dnsclient_unix_test.go:82: goLookupIP should failed: %v lookup > http://golang.org on > > 192.168.1.1:53: no answer from server > > --- FAIL: TestGoogleSRV (60.17 seconds) > > lookup_test.go:24: failed: lookup http://_xmpp-server._tcp.google.com on > > 192.168.1.1:53: no answer from server > > lookup_test.go:27: no results > > FAIL > > > > I fixed the govet issues, fixed a missing resolvConfPath and updated the test. > Should be fixed. > > > perhaps you need to run go tool vet. also looks like your cl has a race > > condition. > > > > WARNING: DATA RACE > > Write by goroutine 350: > > net.loadConfig() > > /home/mikioh/go/src/pkg/net/dnsclient_unix.go:177 +0x217 > > net.TestReloadResolvConfFail() > > /home/mikioh/go/src/pkg/net/dnsclient_unix_test.go:39 +0x125 > > (snip) > > Found 5 data race(s) > > FAIL net 135.153s > > > > I do not manage to reproduce this race with -race. Could you send me the whole > output? > > The line 177 is protected by sync.Once, I'd be interested to see who is reading > it as the same time. > > > > > src/pkg/net/dnsclient_unix.go:159: var cfg struct { > > > > do we really need this indirection? why not change dnsConfig. > > > > > > It is not directly linked to dnsConfig, it reloads it, so I think it is best > > to > > > have a struct here. > > > > seems the new indirection introduces a few race conditions. when running `go test -v -race -cpus 2,4,8 net`, I do have a race in TestDialGoogleIPv4-2, but it seems unrelated and it is also present in tip (without my patches)
Sign in to reply to this message.
I can see that, but thinking on it more - what this does is essentially punishes the 99% use case of not having a broken resolv.conf AND using go dns, which is probably a pretty rare occurance. As it sits, a single goLookupHost calls this same logic(onceLoad, lock, two ors, unlock) a minimum of 3 times, and more in some scenarios. It's something else to think about, I think Mikio wants to make some bigger cleanup in 1.4. As it sits now, we'd have to merge this with the outcome of 75180043. It wouldn't be overly hard as they focus on different pieces, but do touch the same areas so will have to be hand mended. Thanks, Alex On Sat, Apr 5, 2014 at 1:10 PM, <guillaume@charmes.net> wrote: > On 2014/04/05 17:08:03, axaxs wrote: > >> Are we considering for 1.3, or going to wait to merge into other logic >> > fixes > >> pending review(for 1.4, I guess)? >> > > > https://codereview.appspot.com/83690045/diff/160001/src/ > pkg/net/dnsclient_unix.go > >> File src/pkg/net/dnsclient_unix.go (right): >> > > > https://codereview.appspot.com/83690045/diff/160001/src/ > pkg/net/dnsclient_unix.go#newcode284 > >> src/pkg/net/dnsclient_unix.go:284: >> > onceLoadConfig.Do(loadDefaultConfig) > >> Since this calls goLookupIP, which calls lookup, the err should come >> > back from > >> lookup in the end(after minimal allocs, sure). Should we remove this >> > block as > >> you did for CNAME. >> > > > https://codereview.appspot.com/83690045/diff/160001/src/ > pkg/net/dnsclient_unix.go#newcode322 > >> src/pkg/net/dnsclient_unix.go:322: >> > onceLoadConfig.Do(loadDefaultConfig) > >> calls lookup, which does this. See above comment. Can we clean up? >> > > I don't know about 1.3/1.4, from what I understood, we past the feature > freeze for 1.3 :(. > > For the error check, I though about this, but I think we should keep > them to speedup the process in case of error. > > https://codereview.appspot.com/83690045/ >
Sign in to reply to this message.
On 2014/04/05 23:13:43, axaxs wrote: > I can see that, but thinking on it more - what this does is essentially > punishes the 99% use case of not having a broken resolv.conf AND using go > dns, which is probably a pretty rare occurance. As it sits, a single > goLookupHost calls this same logic(onceLoad, lock, two ors, unlock) a > minimum of 3 times, and more in some scenarios. Agreed, this made sense before when an error was doomed to stay forever, now that it can be updated, I'll remove them. > > It's something else to think about, I think Mikio wants to make some bigger > cleanup in 1.4. As it sits now, we'd have to merge this with the outcome > of 75180043. It wouldn't be overly hard as they focus on different pieces, > but do touch the same areas so will have to be hand mended. > I am fairly new to this CL process. If 75180043 gets merged first, I'll update promptly my CL, if 83690045 gets merged first, I can offer help for 75180043. > Thanks, > Alex > > > On Sat, Apr 5, 2014 at 1:10 PM, <mailto:guillaume@charmes.net> wrote: > > > On 2014/04/05 17:08:03, axaxs wrote: > > > >> Are we considering for 1.3, or going to wait to merge into other logic > >> > > fixes > > > >> pending review(for 1.4, I guess)? > >> > > > > > > https://codereview.appspot.com/83690045/diff/160001/src/ > > pkg/net/dnsclient_unix.go > > > >> File src/pkg/net/dnsclient_unix.go (right): > >> > > > > > > https://codereview.appspot.com/83690045/diff/160001/src/ > > pkg/net/dnsclient_unix.go#newcode284 > > > >> src/pkg/net/dnsclient_unix.go:284: > >> > > onceLoadConfig.Do(loadDefaultConfig) > > > >> Since this calls goLookupIP, which calls lookup, the err should come > >> > > back from > > > >> lookup in the end(after minimal allocs, sure). Should we remove this > >> > > block as > > > >> you did for CNAME. > >> > > > > > > https://codereview.appspot.com/83690045/diff/160001/src/ > > pkg/net/dnsclient_unix.go#newcode322 > > > >> src/pkg/net/dnsclient_unix.go:322: > >> > > onceLoadConfig.Do(loadDefaultConfig) > > > >> calls lookup, which does this. See above comment. Can we clean up? > >> > > > > I don't know about 1.3/1.4, from what I understood, we past the feature > > freeze for 1.3 :(. > > > > For the error check, I though about this, but I think we should keep > > them to speedup the process in case of error. > > > > https://codereview.appspot.com/83690045/ > >
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
Getting close! * The implementation looks good, modulo nitpicks below. * I don't see how the tests as written actually test the full reload loop. (And go test -cover agrees.) It seems like what you actually want to do to test is to create a temp file and overwrite it with good/bad DNS configs in turn. As a concrete example, if you comment out "dnserr = nil" in loadConfig (currently line 195), I'd expect a test to fail somewhere, and that doesn't happen right now. * Doesn't look like you've signed a CLA yet -- see the bottom of http://golang.org/doc/contribute.html. https://codereview.appspot.com/83690045/diff/200001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/83690045/diff/200001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:163: mtime time.Time I'd suggest moving mtime above mu, and adding a comment to mu like: "// protects following fields". (Note that mu gets read w/o a mu readlock.) https://codereview.appspot.com/83690045/diff/200001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:165: var dnserr error Seems like this should be part of cfg, since it is protected by mu. https://codereview.appspot.com/83690045/diff/200001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix_test.go (right): https://codereview.appspot.com/83690045/diff/200001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix_test.go:39: loadConfig("testdata/resolv.conf.bad", 1*time.Second, quitCh) Here and below, would (say) 50ms or 100ms be enough? Not sure what the trade-off is, but faster-running tests would be nice.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
The tests are failing for me: $ go test net --- FAIL: TestReloadResolvConfChange (0.57 seconds) dnsclient_unix_test.go:116: goLookupIP should failed but succeded --- FAIL: TestGoogleSRV (0.00 seconds) lookup_test.go:24: failed: lookup _xmpp-server._tcp.google.com: no DNS servers lookup_test.go:27: no results lookup_test.go:33: back door failed: lookup _xmpp-server._tcp.google.com: no DNS servers lookup_test.go:36: back door no results --- FAIL: TestGmailMX (0.00 seconds) lookup_test.go:46: failed: lookup gmail.com: no DNS servers lookup_test.go:49: no results --- FAIL: TestGmailNS (0.00 seconds) lookup_test.go:59: failed: lookup gmail.com: no DNS servers lookup_test.go:62: no results --- FAIL: TestGmailTXT (0.00 seconds) lookup_test.go:72: failed: lookup gmail.com: no DNS servers lookup_test.go:75: no results --- FAIL: TestGoogleDNSAddr (0.00 seconds) lookup_test.go:85: failed: lookup 8.8.8.8.in-addr.arpa.: no DNS servers lookup_test.go:88: no results FAIL FAIL net 21.740s Without this CL, 'go test net' passes. I'd really love to see this included in Go 1.3, but given that the beta is going out today, I'm not sure that this is going to make it, alas. Also, it looks like you haven't signed the CLA -- see the bottom of http://golang.org/doc/contribute.html.
Sign in to reply to this message.
My bad, I ran only my tests, forgot to rerun the whole one. And I did signed the CLA. Do I need to add something to the CL? On 2014/04/14 17:50:39, josharian wrote: > The tests are failing for me: > > $ go test net > --- FAIL: TestReloadResolvConfChange (0.57 seconds) > dnsclient_unix_test.go:116: goLookupIP should failed but succeded > --- FAIL: TestGoogleSRV (0.00 seconds) > lookup_test.go:24: failed: lookup http://_xmpp-server._tcp.google.com: no DNS servers > lookup_test.go:27: no results > lookup_test.go:33: back door failed: lookup http://_xmpp-server._tcp.google.com: no > DNS servers > lookup_test.go:36: back door no results > --- FAIL: TestGmailMX (0.00 seconds) > lookup_test.go:46: failed: lookup http://gmail.com: no DNS servers > lookup_test.go:49: no results > --- FAIL: TestGmailNS (0.00 seconds) > lookup_test.go:59: failed: lookup http://gmail.com: no DNS servers > lookup_test.go:62: no results > --- FAIL: TestGmailTXT (0.00 seconds) > lookup_test.go:72: failed: lookup http://gmail.com: no DNS servers > lookup_test.go:75: no results > --- FAIL: TestGoogleDNSAddr (0.00 seconds) > lookup_test.go:85: failed: lookup 8.8.8.8.in-addr.arpa.: no DNS servers > lookup_test.go:88: no results > FAIL > FAIL net 21.740s > > > Without this CL, 'go test net' passes. > > I'd really love to see this included in Go 1.3, but given that the beta is going > out today, I'm not sure that this is going to make it, alas. > > Also, it looks like you haven't signed the CLA -- see the bottom of > http://golang.org/doc/contribute.html.
Sign in to reply to this message.
> And I did signed the CLA. Do I need to add something to the CL? Ian, would you mind checking on the CLA status? Thanks! > My bad, I ran only my tests, forgot to rerun the whole one. No prob. I also have to apologize, as I missed something (repeatedly!) that would be nice to fix as long as you're making another change: https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Indent_Error_Flow So instead of if fi, err := os.Stat(resolvConfPath); err == nil { // nested if do fi, err := os.Stat(resolvConfPath) if err != nil { continue } // and so on
Sign in to reply to this message.
On Mon, Apr 14, 2014 at 11:03 AM, <josharian@gmail.com> wrote: >> And I did signed the CLA. Do I need to add something to the CL? > > > Ian, would you mind checking on the CLA status? Thanks! Added to AUTHORS/CONTRIBUTORS. Ian
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
I'm afraid that the tests still fail for me: $ go test net --- FAIL: TestReloadResolvConfChange (0.50 seconds) dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], got: [8.8.8.8] FAIL FAIL net 21.618s Running with -cpu=1,2,4,8 yields a different error mixed in as well: --- FAIL: TestReloadResolvConfChange (0.61 seconds) dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], got: [8.8.8.8] --- FAIL: TestReloadResolvConfFail-2 (0.20 seconds) dnsclient_unix_test.go:67: goLookupIP failed: stat testdata/resolv.conf.tmp: no such file or directory --- FAIL: TestReloadResolvConfChange-2 (0.78 seconds) dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], got: [8.8.8.8] --- FAIL: TestReloadResolvConfFail-4 (0.20 seconds) dnsclient_unix_test.go:67: goLookupIP failed: stat testdata/resolv.conf.tmp: no such file or directory --- FAIL: TestReloadResolvConfFail-8 (0.30 seconds) dnsclient_unix_test.go:67: goLookupIP failed: stat testdata/resolv.conf.tmp: no such file or directory --- FAIL: TestReloadResolvConfChange-8 (0.60 seconds) dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], got: [8.8.8.8]
Sign in to reply to this message.
Hmm, weird. My previous patch was time based so obviously racy, but now, everything is sequential. Every pass for me. I'll try with -cpu=1,2,4,8 On 2014/04/15 16:55:09, josharian wrote: > I'm afraid that the tests still fail for me: > > $ go test net > --- FAIL: TestReloadResolvConfChange (0.50 seconds) > dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], > got: [8.8.8.8] > FAIL > FAIL net 21.618s > > > Running with -cpu=1,2,4,8 yields a different error mixed in as well: > > --- FAIL: TestReloadResolvConfChange (0.61 seconds) > dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], > got: [8.8.8.8] > --- FAIL: TestReloadResolvConfFail-2 (0.20 seconds) > dnsclient_unix_test.go:67: goLookupIP failed: stat testdata/resolv.conf.tmp: no > such file or directory > --- FAIL: TestReloadResolvConfChange-2 (0.78 seconds) > dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], > got: [8.8.8.8] > --- FAIL: TestReloadResolvConfFail-4 (0.20 seconds) > dnsclient_unix_test.go:67: goLookupIP failed: stat testdata/resolv.conf.tmp: no > such file or directory > --- FAIL: TestReloadResolvConfFail-8 (0.30 seconds) > dnsclient_unix_test.go:67: goLookupIP failed: stat testdata/resolv.conf.tmp: no > such file or directory > --- FAIL: TestReloadResolvConfChange-8 (0.60 seconds) > dnsclient_unix_test.go:136: Unexpected dns server loaded. Expected: [8.8.4.4], > got: [8.8.8.8]
Sign in to reply to this message.
> Hmm, weird. My previous patch was time based so obviously racy, but now, > everything is sequential. Every pass for me. 'go test -run TestReloadResolvConfChange net' fails about half the time for me. Feel free to ping me off-list (josharian@gmail.com) if you want system information, want me to add extra logging to help you figure it out, etc. -josh
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
On 2014/04/16 16:31:38, creack wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org, mailto:josharian@gmail.com, > mailto:mikioh.mikioh@gmail.com, mailto:alex@lx.lc (cc: mailto:golang-codereviews@googlegroups.com, > mailto:rsc@golang.org), > > Please take another look. $ while true; do sleep 1; go test net; done --- FAIL: TestReloadResolvConfChange (0.60 seconds) dnsclient_unix_test.go:160: Unexpected dns server loaded. Expected: [8.8.4.4], got: [8.8.8.8] FAIL FAIL net 21.283s ok net 21.454s --- FAIL: TestReloadResolvConfChange (0.60 seconds) dnsclient_unix_test.go:160: Unexpected dns server loaded. Expected: [8.8.4.4], got: [8.8.8.8] FAIL FAIL net 22.423s --- FAIL: TestReloadResolvConfChange (0.60 seconds) dnsclient_unix_test.go:160: Unexpected dns server loaded. Expected: [8.8.4.4], got: [8.8.8.8] I'm swamped at the moment, but if you can wait a day or three, I'll look closer and see whether I can't diagnose.
Sign in to reply to this message.
TestReloadResolvConfFail is failing for me because cfg.mtime doesn't get reset; an old cfg.mtime prevented an intended reload. This is dependent on the timing of the tests and the mtime resolution, either of which might explain why I can reproduce and you can't. I think that the right fix for this is for mtime to just be a local variable in loadConfig. TestReloadResolvConfChange is failing for me because of a similar reason. The mtime of resolv.conf.tmp doesn't change between copying resolv.conf.good and copying resolv.conf.good.2; both copies happen within the mtime resolution, which is 1s on my system. I guess the robust way to fix this is to poll until the mtime has changed. The simpler, probably better, way is to trust that no systems will have an mtime resolution coarser than 1s and insert 1s sleeps (sad). Since this change will require adding it everywhere, and there's already a fair amount of duplication (filling up the buffered chan), it might be nice to refactor a tiny bit. Also, I believe that tests run serially unless explicitly requested otherwise, so you probably don't need lockTestReloadResolvConfFail and lockTestReloadResolvConfChange.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
Sign in to reply to this message.
LGTM but as I'm now too familiar with this code, I'd like another reviewer to take a look as well Also, I'd be great to get a go/no-go call for Go 1.3 from rsc or iant.
Sign in to reply to this message.
Oh, and thanks for your tenacity in seeing this through. Much appreciated. -josh
Sign in to reply to this message.
looks like introducing "type resolvConf struct" and using it not only for reading resolv.conf but controlling builtin dns resolver traffic make sense, but it will happen in go1.4. https://codereview.appspot.com/83690045/diff/420001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/83690045/diff/420001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:159: var cfg struct { please rename to more appropriate one, it's not a dnsConfig struct. https://codereview.appspot.com/83690045/diff/420001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix_test.go (right): https://codereview.appspot.com/83690045/diff/420001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix_test.go:5: // +build darwin dragonfly freebsd linux netbsd openbsd no solaris?
Sign in to reply to this message.
R=iant@golang.org (assigned by mikioh.mikioh@gmail.com)
Sign in to reply to this message.
Sorry it has been a while since I've looked at this myself. It's late here so please excuse the (perhaps stupid) question, but what is the quit chan doing here? I see its purpose in the code, but I don't see when/how it could ever really be used. Thanks, Alex On Tue, Apr 29, 2014 at 8:46 PM, <mikioh.mikioh@gmail.com> wrote: > looks like introducing "type resolvConf struct" and using it not only > for reading resolv.conf but controlling builtin dns resolver traffic > make sense, but it will happen in go1.4. > > > https://codereview.appspot.com/83690045/diff/420001/src/ > pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/83690045/diff/420001/src/ > pkg/net/dnsclient_unix.go#newcode159 > src/pkg/net/dnsclient_unix.go:159: var cfg struct { > please rename to more appropriate one, it's not a dnsConfig struct. > > https://codereview.appspot.com/83690045/diff/420001/src/ > pkg/net/dnsclient_unix_test.go > File src/pkg/net/dnsclient_unix_test.go (right): > > https://codereview.appspot.com/83690045/diff/420001/src/ > pkg/net/dnsclient_unix_test.go#newcode5 > src/pkg/net/dnsclient_unix_test.go:5: // +build darwin dragonfly freebsd > linux netbsd openbsd > no solaris? > > https://codereview.appspot.com/83690045/ >
Sign in to reply to this message.
The quit chan is used only in tests. It allows the tests to start and stop the goroutine which is perpetual otherwise. On 2014/04/30 02:05:50, axaxs wrote: > Sorry it has been a while since I've looked at this myself. It's late here > so please excuse the (perhaps stupid) question, but what is the quit chan > doing here? I see its purpose in the code, but I don't see when/how it > could ever really be used. > > Thanks, > Alex > > > > > On Tue, Apr 29, 2014 at 8:46 PM, <mailto:mikioh.mikioh@gmail.com> wrote: > > > looks like introducing "type resolvConf struct" and using it not only > > for reading resolv.conf but controlling builtin dns resolver traffic > > make sense, but it will happen in go1.4. > > > > > > https://codereview.appspot.com/83690045/diff/420001/src/ > > pkg/net/dnsclient_unix.go > > File src/pkg/net/dnsclient_unix.go (right): > > > > https://codereview.appspot.com/83690045/diff/420001/src/ > > pkg/net/dnsclient_unix.go#newcode159 > > src/pkg/net/dnsclient_unix.go:159: var cfg struct { > > please rename to more appropriate one, it's not a dnsConfig struct. > > > > https://codereview.appspot.com/83690045/diff/420001/src/ > > pkg/net/dnsclient_unix_test.go > > File src/pkg/net/dnsclient_unix_test.go (right): > > > > https://codereview.appspot.com/83690045/diff/420001/src/ > > pkg/net/dnsclient_unix_test.go#newcode5 > > src/pkg/net/dnsclient_unix_test.go:5: // +build darwin dragonfly freebsd > > linux netbsd openbsd > > no solaris? > > > > https://codereview.appspot.com/83690045/ > >
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=afc520948686 *** net: detect changes to /etc/resolv.conf. Implement the changes as suggested by rsc. Fixes issue 6670. LGTM=josharian, iant R=golang-codereviews, iant, josharian, mikioh.mikioh, alex, gobot CC=golang-codereviews, rsc https://codereview.appspot.com/83690045 Committer: Ian Lance Taylor <iant@golang.org>
Sign in to reply to this message.
The third argument of loadConfig() is always nil, so the code to read from it is basically dead code. I think we should remove it.
Sign in to reply to this message.
On Thu, May 15, 2014 at 10:58 AM, <ruiu@google.com> wrote: > The third argument of loadConfig() is always nil, so the code to > read from it is basically dead code. I think we should remove it. it's already asked by Alex (axaxs); it's a hook for testing. i'm fine either for now, because axaxs and i agree that we'll do cleanup of builtin dns resolver stuff in go1.4.
Sign in to reply to this message.
If you have a better idea for testing, I'd love to remove that parameter On Wednesday, May 14, 2014, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > On Thu, May 15, 2014 at 10:58 AM, <ruiu@google.com <javascript:;>> wrote: > > > The third argument of loadConfig() is always nil, so the code to > > read from it is basically dead code. I think we should remove it. > > it's already asked by Alex (axaxs); it's a hook for testing. i'm fine > either for now, because axaxs and i agree that we'll do cleanup of > builtin dns resolver stuff in go1.4. > -- -- Guillaume J. Charmes about.me/guillaumecharmes [image: Guillaume J. Charmes on about.me] <http://about.me/guillaumecharmes>
Sign in to reply to this message.
if i were you, i'd use resolveconf instead of keeping fake resolvconf-ish stuff. the test case already depends on network logical facilities, physical attachments and external network connectivities. so one more additional thing doesn't matter. also it'd provide a bit better test coverage. On Thu, May 15, 2014 at 11:24 AM, Guillaume J. Charmes < guillaume@charmes.net> wrote: > If you have a better idea for testing, I'd love to remove that parameter > > > On Wednesday, May 14, 2014, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > >> On Thu, May 15, 2014 at 10:58 AM, <ruiu@google.com> wrote: >> >> > The third argument of loadConfig() is always nil, so the code to >> > read from it is basically dead code. I think we should remove it. >> >> it's already asked by Alex (axaxs); it's a hook for testing. i'm fine >> either for now, because axaxs and i agree that we'll do cleanup of >> builtin dns resolver stuff in go1.4. >> > > > -- > -- > > Guillaume J. Charmes > about.me/guillaumecharmes > [image: Guillaume J. Charmes on about.me] > <http://about.me/guillaumecharmes> > >
Sign in to reply to this message.
On Wed, May 14, 2014 at 7:09 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > On Thu, May 15, 2014 at 10:58 AM, <ruiu@google.com> wrote: > > > The third argument of loadConfig() is always nil, so the code to > > read from it is basically dead code. I think we should remove it. > > it's already asked by Alex (axaxs); it's a hook for testing. i'm fine > either for now, because axaxs and i agree that we'll do cleanup of > builtin dns resolver stuff in go1.4. > I see. It makes sense. As you might have already discussed, you might want to do the same goroutine file polling operation on /etc/hosts as well. I'm curious why we use a goroutine for /etc/resolv.conf but don't for /etc/hosts.
Sign in to reply to this message.
On Thu, May 15, 2014 at 1:31 PM, Rui Ueyama <ruiu@google.com> wrote: > As you might have already discussed, you might want to do the same goroutine > file polling operation on /etc/hosts as well. I'm curious why we use a > goroutine for /etc/resolv.conf but don't for /etc/hosts. because we've been waiting for fsevnet-like stuff long time. but in go1.4, eventually... hope so.
Sign in to reply to this message.
|