From a6bebd85ed4361b1cda72db524124724dbf1f2eb Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 16 Jun 2015 15:19:57 +0100 Subject: [PATCH 1/7] Remove 'running' member that triggers a race warning and isn't necessary. --- nameserver/mdns_client.go | 16 ++++------------ nameserver/mdns_server.go | 8 -------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/nameserver/mdns_client.go b/nameserver/mdns_client.go index 2ab57c99..5cc33ba2 100644 --- a/nameserver/mdns_client.go +++ b/nameserver/mdns_client.go @@ -97,7 +97,6 @@ type MDNSClient struct { addr *net.UDPAddr inflight map[string]*inflightQuery actionChan chan<- MDNSAction - running bool } type mDNSQueryInfo struct { @@ -109,15 +108,10 @@ type mDNSQueryInfo struct { func NewMDNSClient() (*MDNSClient, error) { return &MDNSClient{ addr: ipv4Addr, - running: false, inflight: make(map[string]*inflightQuery)}, nil } func (c *MDNSClient) Start(ifi *net.Interface) (err error) { - if c.running { - return nil - } - if c.conn, err = net.ListenUDP("udp4", &net.UDPAddr{IP: net.IPv4zero, Port: 0}); err != nil { return err } @@ -144,9 +138,7 @@ func (c *MDNSClient) Start(ifi *net.Interface) (err error) { } func (c *MDNSClient) Stop() error { - if c.running { - c.actionChan <- nil - } + c.actionChan <- nil return nil } @@ -272,13 +264,13 @@ func (c *MDNSClient) checkInFlightQueries() time.Duration { func (c *MDNSClient) actorLoop(actionChan <-chan MDNSAction) { timer := time.NewTimer(MaxDuration) run := func() { timer.Reset(c.checkInFlightQueries()) } - c.running = true - for c.running { +loop: + for { select { case action := <-actionChan: if action == nil { c.listener.Shutdown() - c.running = false + break loop } else { action() run() diff --git a/nameserver/mdns_server.go b/nameserver/mdns_server.go index d95a6c1b..4a013a4f 100644 --- a/nameserver/mdns_server.go +++ b/nameserver/mdns_server.go @@ -14,7 +14,6 @@ type MDNSServer struct { zone Zone allowLocal bool ttl int - running bool } // Create a new mDNS server @@ -43,11 +42,6 @@ func (s *MDNSServer) addrIsLocal(testaddr net.Addr) bool { // Start the mDNS server func (s *MDNSServer) Start(ifi *net.Interface) (err error) { - // skip double initialization - if s.running { - return nil - } - // This is a bit of a kludge - per the RFC we should send responses from 5353, but that doesn't seem to work s.sendconn, err = net.ListenUDP("udp4", &net.UDPAddr{IP: net.IPv4zero, Port: 0}) if err != nil { @@ -94,13 +88,11 @@ func (s *MDNSServer) Start(ifi *net.Interface) (err error) { Handler: mux, } go s.srv.ActivateAndServe() - s.running = true return err } // Stop the mDNS server func (s *MDNSServer) Stop() error { - s.running = false return s.srv.Shutdown() } From 1f92ae6ba8031623579a1d9659d3ddd8b3639933 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 16 Jun 2015 15:50:17 +0100 Subject: [PATCH 2/7] Swap the order in which two goroutines are started, since one relies on the other. --- nameserver/mdns_client.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/nameserver/mdns_client.go b/nameserver/mdns_client.go index 5cc33ba2..0be13fbd 100644 --- a/nameserver/mdns_client.go +++ b/nameserver/mdns_client.go @@ -128,12 +128,13 @@ func (c *MDNSClient) Start(ifi *net.Interface) (err error) { } } - c.listener = &dns.Server{Unsafe: true, PacketConn: multicast, Handler: dns.HandlerFunc(handleMDNS)} - go c.listener.ActivateAndServe() - actionChan := make(chan MDNSAction, MailboxSize) c.actionChan = actionChan go c.actorLoop(actionChan) + + c.listener = &dns.Server{Unsafe: true, PacketConn: multicast, Handler: dns.HandlerFunc(handleMDNS)} + go c.listener.ActivateAndServe() + return nil } From e68364cb3b6a1ef9032f623c575b384f84b453c3 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 16 Jun 2015 16:15:56 +0100 Subject: [PATCH 3/7] Don't overwrite recvChan; it just creates a race condition --- nameserver/mdns_server_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/nameserver/mdns_server_test.go b/nameserver/mdns_server_test.go index bd6723a8..2cd18e15 100644 --- a/nameserver/mdns_server_test.go +++ b/nameserver/mdns_server_test.go @@ -60,7 +60,6 @@ func TestServerSimpleQuery(t *testing.T) { receivedAddrs = make([]net.IP, 0) receivedName = "" receivedCount = 0 - recvChan = make(chan interface{}) m := new(dns.Msg) m.SetQuestion(name, querytype) From 657133db50f2ba049a765fb240ae9df84062f123 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 16 Jun 2015 16:25:12 +0100 Subject: [PATCH 4/7] Move the counting of results from DNS goroutine to test goroutine to avoid race-condition --- nameserver/mdns_server_test.go | 36 +++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/nameserver/mdns_server_test.go b/nameserver/mdns_server_test.go index 2cd18e15..29b72cef 100644 --- a/nameserver/mdns_server_test.go +++ b/nameserver/mdns_server_test.go @@ -41,16 +41,7 @@ func TestServerSimpleQuery(t *testing.T) { if len(r.Answer) > 0 { t.Logf("Received %d answer(s)", len(r.Answer)) for _, answer := range r.Answer { - switch rr := answer.(type) { - case *dns.A: - t.Logf("... A:\n%+v", rr) - receivedAddrs = append(receivedAddrs, rr.A) - receivedCount++ - case *dns.PTR: - t.Logf("... PTR:\n%+v", rr) - receivedName = rr.Ptr - receivedCount++ - } + recvChan <- answer } recvChan <- "ok" } @@ -73,12 +64,25 @@ func TestServerSimpleQuery(t *testing.T) { wt.AssertNoErr(t, err) Debug.Printf("Waiting for response") - select { - case <-recvChan: - return - case <-time.After(100 * time.Millisecond): - Debug.Printf("Timeout while waiting for response") - return + for { + select { + case x := <-recvChan: + switch rr := x.(type) { + case *dns.A: + t.Logf("... A:\n%+v", rr) + receivedAddrs = append(receivedAddrs, rr.A) + receivedCount++ + case *dns.PTR: + t.Logf("... PTR:\n%+v", rr) + receivedName = rr.Ptr + receivedCount++ + case string: + return + } + case <-time.After(100 * time.Millisecond): + Debug.Printf("Timeout while waiting for response") + return + } } } From 90fb201a51db3e1a0958d46db9ecfaa2e8fc0d51 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 16 Jun 2015 16:45:22 +0100 Subject: [PATCH 5/7] Copy the DNS entry going into the cache, to avoid races. The shuffleAnswers() call ends up acting on the same data, so it seems simplest to resolve ownership arguments by copying. --- nameserver/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nameserver/cache.go b/nameserver/cache.go index c65824ed..4af548c8 100644 --- a/nameserver/cache.go +++ b/nameserver/cache.go @@ -155,7 +155,7 @@ func (e *cacheEntry) setReply(reply *dns.Msg, ttl int, flags uint8, now time.Tim e.putTime = now if reply != nil { - e.reply = *reply + e.reply = *reply.Copy() e.ReplyLen = reply.Len() } From 1b57a247894bdd8f1ea6f36ce8901ae82b8bce9d Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 16 Jun 2015 16:51:19 +0100 Subject: [PATCH 6/7] Don't re-initialise log writers unless the type changed. This avoids race-conditions in unit tests where the initialisation is on a different goroutine to the logging. --- common/logging.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/logging.go b/common/logging.go index 6c3f5627..8def1602 100644 --- a/common/logging.go +++ b/common/logging.go @@ -19,6 +19,7 @@ var ( Info *log.Logger Warning *log.Logger Error *log.Logger + debugF bool ) func InitLogging(debugHandle io.Writer, @@ -33,6 +34,10 @@ func InitLogging(debugHandle io.Writer, } func InitDefaultLogging(debug bool) { + if debug == debugF { + return + } + debugF = debug debugOut := ioutil.Discard if debug { debugOut = os.Stderr From 9636675c2cac020d6484c5a1f258ad7080a06c95 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 16 Jun 2015 16:58:18 +0100 Subject: [PATCH 7/7] Split part of DNSServer.Start() to a separate ActivateAndServe() Specifically, move the part that blocks, and call that as necessary. This means Start() can run on the caller's goroutine and avoid race conditions. --- nameserver/server.go | 4 +++- nameserver/server_cache_test.go | 12 +++++++++--- nameserver/server_test.go | 8 ++++++-- prog/weavedns/main.go | 1 + 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/nameserver/server.go b/nameserver/server.go index 336b863f..a7b84bc7 100644 --- a/nameserver/server.go +++ b/nameserver/server.go @@ -218,7 +218,10 @@ func (s *DNSServer) Start() error { s.tcpSrv = &dns.Server{Listener: l, Handler: s.createMux(protTCP), ReadTimeout: s.readTimeout} s.listenersWg.Add(2) + return nil +} +func (s *DNSServer) ActivateAndServe() { go func() { defer s.listenersWg.Done() @@ -241,7 +244,6 @@ func (s *DNSServer) Start() error { s.listenersWg.Wait() Info.Printf("[dns] Server exiting...") - return nil } // Return status string diff --git a/nameserver/server_cache_test.go b/nameserver/server_cache_test.go index 5b366928..65f233df 100644 --- a/nameserver/server_cache_test.go +++ b/nameserver/server_cache_test.go @@ -69,7 +69,9 @@ func TestServerDbCacheInvalidation(t *testing.T) { MaxAnswers: 4, }) wt.AssertNoErr(t, err) - go srv.Start() + err = srv.Start() + wt.AssertNoErr(t, err) + go srv.ActivateAndServe() defer srv.Stop() time.Sleep(100 * time.Millisecond) // Allow server goroutine to start @@ -202,7 +204,9 @@ func TestServerCacheExpiration(t *testing.T) { CacheNegLocalTTL: negativeLocalTTL, }) wt.AssertNoErr(t, err) - go srv.Start() + err = srv.Start() + wt.AssertNoErr(t, err) + go srv.ActivateAndServe() defer srv.Stop() time.Sleep(100 * time.Millisecond) // Allow server goroutine to start @@ -287,7 +291,9 @@ func TestServerCacheRefresh(t *testing.T) { MaxAnswers: 4, }) wt.AssertNoErr(t, err) - go srv.Start() + err = srv.Start() + wt.AssertNoErr(t, err) + go srv.ActivateAndServe() defer srv.Stop() time.Sleep(100 * time.Millisecond) // Allow sever goroutine to start diff --git a/nameserver/server_test.go b/nameserver/server_test.go index 360222f4..feba9997 100644 --- a/nameserver/server_test.go +++ b/nameserver/server_test.go @@ -89,7 +89,9 @@ func TestUDPDNSServer(t *testing.T) { ListenReadTimeout: testSocketTimeout, }) wt.AssertNoErr(t, err) - go srv.Start() + err = srv.Start() + wt.AssertNoErr(t, err) + go srv.ActivateAndServe() defer srv.Stop() time.Sleep(100 * time.Millisecond) // Allow sever goroutine to start @@ -195,7 +197,9 @@ func TestTCPDNSServer(t *testing.T) { ListenReadTimeout: testSocketTimeout, }) wt.AssertNoErr(t, err) - go srv.Start() + err = srv.Start() + wt.AssertNoErr(t, err) + go srv.ActivateAndServe() defer srv.Stop() time.Sleep(100 * time.Millisecond) // Allow sever goroutine to start diff --git a/prog/weavedns/main.go b/prog/weavedns/main.go index f22ef8e5..21193da6 100644 --- a/prog/weavedns/main.go +++ b/prog/weavedns/main.go @@ -182,4 +182,5 @@ func main() { if err != nil { Error.Fatal("[main] Failed to start the WeaveDNS server: ", err) } + srv.ActivateAndServe() }