From 42032584b59d14550e099867f289344bce0f149c Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Tue, 9 Jan 2018 12:42:27 -0600 Subject: [PATCH 1/2] fix a number of data races datadog statsd client contained a race condition - was fixed in master Server.Logger contained a race where multiple loggers could write to the same output io.Writer TestMain_FrameRestore contained a race where it tried to change a cluster's nodes while it was running (which conflicted with antiEntropy reading that state). --- Gopkg.lock | 10 +++++----- Gopkg.toml | 8 ++++++++ server.go | 5 +++-- server/server_test.go | 35 ++++++++++++++--------------------- 4 files changed, 30 insertions(+), 28 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index ed772ef39..96a72b8f2 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -14,10 +14,10 @@ revision = "39b0596a2da3c92787b3319c6b5425a474b4e0da" [[projects]] + branch = "master" name = "github.com/DataDog/datadog-go" packages = ["statsd"] - revision = "0ddda6bee21174ef6c4873647cb0d6ec9cba996f" - version = "1.1.0" + revision = "4d2e5696ebe914940bd7459d2266fb7d555ea1b7" [[projects]] branch = "master" @@ -46,8 +46,8 @@ [[projects]] name = "github.com/gogo/protobuf" packages = ["proto"] - revision = "342cbe0a04158f6dcb03ca0079991a51a4248c02" - version = "v0.5" + revision = "100ba4e885062801d56799d78530b73b178a78f3" + version = "v0.4" [[projects]] branch = "master" @@ -238,6 +238,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "75badb0bcc3bb356b04af17979e0af61b4b66c5e0a483f09e39cf1f9b5e5de2c" + inputs-digest = "d7c279ee1c617ec26e329979b5f92021287ede9701ff41e57c1fecad92ec6b51" solver-name = "gps-cdcl" solver-version = 1 diff --git a/Gopkg.toml b/Gopkg.toml index 7ffa67a2f..50eddac23 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -1,3 +1,11 @@ # This file intentionally left blank as all needed dependencies are imported by # the project and thus tracked by `dep`. # See https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md for details. + + +[[constraint]] + # Required: the root import path of the project being constrained. + name = "github.com/DataDog/datadog-go" + # Recommended: the version constraint to enforce for the project. + # Only one of "branch", "version" or "revision" can be specified. + branch = "master" diff --git a/server.go b/server.go index ffaad0b71..cb4cab7bd 100644 --- a/server.go +++ b/server.go @@ -80,6 +80,7 @@ type Server struct { MaxWritesPerRequest int LogOutput io.Writer + logger *log.Logger defaultClient InternalClient } @@ -104,9 +105,9 @@ func NewServer() *Server { LogOutput: os.Stderr, } + s.logger = log.New(s.LogOutput, "", log.LstdFlags) s.Handler.Holder = s.Holder - return s } @@ -251,7 +252,7 @@ func GetHTTPClient(t *tls.Config) *http.Client { } // Logger returns a logger that writes to LogOutput -func (s *Server) Logger() *log.Logger { return log.New(s.LogOutput, "", log.LstdFlags) } +func (s *Server) Logger() *log.Logger { return s.logger } func (s *Server) monitorAntiEntropy() { ticker := time.NewTicker(s.AntiEntropyInterval) diff --git a/server/server_test.go b/server/server_test.go index 28886bc67..ba3d66108 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -278,24 +278,17 @@ func TestMain_SetColumnAttrsWithColumnOption(t *testing.T) { // Ensure program can set bits on one cluster and then restore to a second cluster. func TestMain_FrameRestore(t *testing.T) { m0 := MustRunMain() - defer m0.Close() - - m1 := MustRunMain() - defer m1.Close() - - // Update cluster config. - m0.Server.Cluster.Nodes = []*pilosa.Node{ - {Scheme: "http", Host: m0.Server.URI.HostPort()}, - {Scheme: "http", Host: m1.Server.URI.HostPort()}, - } - m1.Server.Cluster.Nodes = m0.Server.Cluster.Nodes + // TODO: this test used to start a two node cluster, but there was a race + // condition with anti-entropy. We need some general code for starting up + // arbitrarily sized Pilosa clusters for testing, and then we should + // re-instate the multi-node nature of this test. // Create frames. client := m0.Client() if err := client.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { - t.Fatal(err) + t.Fatal("create index:", err) } else if err := client.CreateFrame(context.Background(), "i", "f", pilosa.FrameOptions{}); err != nil { - t.Fatal(err) + t.Fatal("create frame:", err) } // Write data on first cluster. @@ -308,12 +301,12 @@ func TestMain_FrameRestore(t *testing.T) { SetBit(rowID=1, frame="f", columnID=600000) SetBit(rowID=1, frame="f", columnID=800000) `); err != nil { - t.Fatal(err) + t.Fatal("setting bits:", err) } // Query row on first cluster. if res, err := m0.Query("i", "", `Bitmap(rowID=1, frame="f")`); err != nil { - t.Fatal(err) + t.Fatal("bitmap query:", err) } else if res != `{"results":[{"attrs":{},"bits":[100,1000,100000,200000,400000,600000,800000]}]}`+"\n" { t.Fatalf("unexpected result: %s", res) } @@ -325,20 +318,20 @@ func TestMain_FrameRestore(t *testing.T) { // Import from first cluster. client, err := pilosa.NewInternalHTTPClient(m2.Server.URI.HostPort(), pilosa.GetHTTPClient(nil)) if err != nil { - t.Fatal(err) + t.Fatal("new client:", err) } else if err := m2.Client().CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { - t.Fatal(err) + t.Fatal("create new index:", err) } else if err := m2.Client().CreateFrame(context.Background(), "i", "f", pilosa.FrameOptions{}); err != nil { - t.Fatal(err) + t.Fatal("create new frame:", err) } else if err := client.RestoreFrame(context.Background(), m0.Server.URI.HostPort(), "i", "f"); err != nil { - t.Fatal(err) + t.Fatal("restore frame:", err) } // Query row on second cluster. if res, err := m2.Query("i", "", `Bitmap(rowID=1, frame="f")`); err != nil { - t.Fatal(err) + t.Fatal("another bitmap query:", err) } else if res != `{"results":[{"attrs":{},"bits":[100,1000,100000,200000,400000,600000,800000]}]}`+"\n" { - t.Fatalf("unexpected result: %s", res) + t.Fatalf("2unexpected result: %s", res) } } From 55de0402d250845cda454b842aed24caf0b00796 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Tue, 9 Jan 2018 13:50:40 -0600 Subject: [PATCH 2/2] convert some locks to rlocks --- frame.go | 4 ++-- holder.go | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/frame.go b/frame.go index 3da55cc93..e6792f4c0 100644 --- a/frame.go +++ b/frame.go @@ -533,8 +533,8 @@ func (f *Frame) view(name string) *View { return f.views[name] } // Views returns a list of all views in the frame. func (f *Frame) Views() []*View { - f.mu.Lock() - defer f.mu.Unlock() + f.mu.RLock() + defer f.mu.RUnlock() other := make([]*View, 0, len(f.views)) for _, view := range f.views { diff --git a/holder.go b/holder.go index 7cbb8ca42..416ddc232 100644 --- a/holder.go +++ b/holder.go @@ -196,15 +196,14 @@ func (h *Holder) index(name string) *Index { return h.indexes[name] } // Indexes returns a list of all indexes in the holder. func (h *Holder) Indexes() []*Index { - h.mu.Lock() - defer h.mu.Unlock() - + h.mu.RLock() a := make([]*Index, 0, len(h.indexes)) for _, index := range h.indexes { a = append(a, index) } - sort.Sort(indexSlice(a)) + h.mu.RUnlock() + sort.Sort(indexSlice(a)) return a }