query: fix deadlock in endpointset (thanos-io#4795)

Avoid RLock()ing twice as described here: thanos-io#4766 (comment) (due to https://stackoverflow.com/questions/30547916/goroutine-blocks-when-calling-rwmutex-rlock-twice-after-an-rwmutex-unlock/30549188). Fix it by removing HasClients() and simply changing it with `er.clients != nil`. Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS · Oct 22, 2021 · a6279e9 · a6279e9
1 parent a3ce6be
commit a6279e9
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 12 deletions.
diff --git a/pkg/query/endpointset.go b/pkg/query/endpointset.go
@@ -676,32 +676,25 @@ func (er *endpointRef) ComponentType() component.Component {
 	return component.FromString(er.metadata.ComponentType)
 }
 
-func (er *endpointRef) HasClients() bool {
-	er.mtx.RLock()
-	defer er.mtx.RUnlock()
-
-	return er.clients != nil
-}
-
 func (er *endpointRef) HasStoreAPI() bool {
 	er.mtx.RLock()
 	defer er.mtx.RUnlock()
 
-	return er.HasClients() && er.clients.store != nil
+	return er.clients != nil && er.clients.store != nil
 }
 
 func (er *endpointRef) HasRulesAPI() bool {
 	er.mtx.RLock()
 	defer er.mtx.RUnlock()
 
-	return er.HasClients() && er.clients.rule != nil
+	return er.clients != nil && er.clients.rule != nil
 }
 
 func (er *endpointRef) HasTargetsAPI() bool {
 	er.mtx.RLock()
 	defer er.mtx.RUnlock()
 
-	return er.HasClients() && er.clients.target != nil
+	return er.clients != nil && er.clients.target != nil
 }
 
 func (er *endpointRef) HasQueryAPI() bool {
@@ -715,14 +708,14 @@ func (er *endpointRef) HasMetricMetadataAPI() bool {
 	er.mtx.RLock()
 	defer er.mtx.RUnlock()
 
-	return er.HasClients() && er.clients.metricMetadata != nil
+	return er.clients != nil && er.clients.metricMetadata != nil
 }
 
 func (er *endpointRef) HasExemplarsAPI() bool {
 	er.mtx.RLock()
 	defer er.mtx.RUnlock()
 
-	return er.HasClients() && er.clients.exemplar != nil
+	return er.clients != nil && er.clients.exemplar != nil
 }
 
 func (er *endpointRef) LabelSets() []labels.Labels {

diff --git a/pkg/query/endpointset_test.go b/pkg/query/endpointset_test.go
@@ -12,6 +12,7 @@ import (
 	"testing"
 	"time"
 
+	"golang.org/x/sync/errgroup"
 	"google.golang.org/grpc"
 
 	"github.com/pkg/errors"
@@ -1123,3 +1124,47 @@ func assertRegisteredAPIs(t *testing.T, expectedAPIs *APIs, er *endpointRef) {
 	testutil.Equals(t, expectedAPIs.metricMetadata, er.HasMetricMetadataAPI())
 	testutil.Equals(t, expectedAPIs.exemplars, er.HasExemplarsAPI())
 }
+
+// Regression test for: https://github.com/thanos-io/thanos/issues/4766.
+func TestDeadlockLocking(t *testing.T) {
+	t.Parallel()
+
+	mockEndpointRef := &endpointRef{
+		addr: "mockedStore",
+		metadata: &endpointMetadata{
+			&infopb.InfoResponse{},
+		},
+		clients: &endpointClients{},
+	}
+
+	g := &errgroup.Group{}
+	deadline := time.Now().Add(3 * time.Second)
+
+	g.Go(func() error {
+		for {
+			if time.Now().After(deadline) {
+				break
+			}
+			mockEndpointRef.Update(&endpointMetadata{
+				InfoResponse: &infopb.InfoResponse{},
+			})
+		}
+		return nil
+	})
+
+	g.Go(func() error {
+		for {
+			if time.Now().After(deadline) {
+				break
+			}
+			mockEndpointRef.HasStoreAPI()
+			mockEndpointRef.HasExemplarsAPI()
+			mockEndpointRef.HasMetricMetadataAPI()
+			mockEndpointRef.HasRulesAPI()
+			mockEndpointRef.HasTargetsAPI()
+		}
+		return nil
+	})
+
+	testutil.Ok(t, g.Wait())
+}