From 758d78858038edc22aaa186f7d9b6e5ebb46c45c Mon Sep 17 00:00:00 2001
From: Yuxuan Li <yuxuanli@google.com>
Date: Mon, 18 Mar 2019 15:49:30 -0700
Subject: [PATCH] update: include no resolver service config case and fix
 reviews

---
 clientconn.go      | 10 +++++++---
 clientconn_test.go | 41 +++++++++++++++++++++++++++++++++--------
 service_config.go  |  9 ++++++---
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/clientconn.go b/clientconn.go
index 326af66c317d..34c4da29d69b 100644
--- a/clientconn.go
+++ b/clientconn.go
@@ -478,6 +478,11 @@ func (cc *ClientConn) handleResolvedAddrs(addrs []resolver.Address, err error) {
 		return
 	}
 
+	// If resolver does not return service config, apply the default service config if available.
+	if cc.scRaw == nil && cc.dopts.defaultServiceConfig != nil {
+		cc.applyServiceConfig(cc.dopts.defaultServiceConfig, cc.dopts.defaultServiceConfigRaw)
+	}
+
 	cc.curAddresses = addrs
 	cc.firstResolveEvent.Fire()
 
@@ -762,6 +767,8 @@ func (cc *ClientConn) getTransport(ctx context.Context, failfast bool, method st
 // handleServiceConfig parses the service config string in JSON format to Go native
 // struct ServiceConfig, and store both the struct and the JSON string in ClientConn.
 func (cc *ClientConn) handleServiceConfig(js string) error {
+	cc.mu.Lock()
+	defer cc.mu.Unlock()
 	if cc.dopts.disableServiceConfig {
 		if cc.dopts.defaultServiceConfig == nil {
 			return nil
@@ -798,12 +805,10 @@ func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error {
 			Severity: channelz.CtINFO,
 		})
 	}
-	cc.mu.Lock()
 	// Check if the ClientConn is already closed. Some fields (e.g.
 	// balancerWrapper) are set to nil when closing the ClientConn, and could
 	// cause nil pointer panic if we don't have this check.
 	if cc.conns == nil {
-		cc.mu.Unlock()
 		return nil
 	}
 	cc.scRaw = &js
@@ -835,7 +840,6 @@ func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, js string) error {
 		}
 	}
 
-	cc.mu.Unlock()
 	return nil
 }
 
diff --git a/clientconn_test.go b/clientconn_test.go
index eac3f3170aec..41a447f4dcc2 100644
--- a/clientconn_test.go
+++ b/clientconn_test.go
@@ -1245,11 +1245,10 @@ func setMinConnectTimeout(newMin time.Duration) (cleanup func()) {
 	}
 }
 
-func (s) TestDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T) {
+func (s) TestDefaultServiceConfig(t *testing.T) {
 	r, cleanup := manual.GenerateAndRegisterManualResolver()
 	defer cleanup()
 	addr := r.Scheme() + ":///non.existent"
-	r.InitialAddrs([]resolver.Address{{Addr: addr}})
 	js := `{
     "methodConfig": [
         {
@@ -1263,12 +1262,11 @@ func (s) TestDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T)
         }
     ]
 }`
-	cc, err := Dial(addr, WithInsecure(), WithDisableServiceConfig(), WithDefaultServiceConfig(js))
-	if err != nil {
-		t.Fatalf("Dial(%s, _) = _, %v, want _, <nil>", addr, err)
-	}
-	defer cc.Close()
-	r.NewServiceConfig("")
+	testDefaultServiceConfigWhenResolverServiceConfigDisabled(t, r, addr, js)
+	testDefaultServiceConfigWhenResolverDoesNotReturnServiceConfig(t, r, addr, js)
+}
+
+func verifyWaitForReadyEqualsTrue(cc *ClientConn) bool {
 	var i int
 	for i = 0; i < 10; i++ {
 		mc := cc.GetMethodConfig("/foo/bar")
@@ -1278,6 +1276,33 @@ func (s) TestDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T)
 		time.Sleep(100 * time.Millisecond)
 	}
 	if i == 10 {
+		return false
+	}
+	return true
+}
+
+func testDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T, r resolver.Resolver, addr string, js string) {
+	cc, err := Dial(addr, WithInsecure(), WithDisableServiceConfig(), WithDefaultServiceConfig(js))
+	if err != nil {
+		t.Fatalf("Dial(%s, _) = _, %v, want _, <nil>", addr, err)
+	}
+	defer cc.Close()
+	// Resolver service config gets ignored since resolver service config is disabled.
+	r.(*manual.Resolver).NewServiceConfig("")
+	r.(*manual.Resolver).NewAddress([]resolver.Address{{Addr: addr}})
+	if !verifyWaitForReadyEqualsTrue(cc) {
+		t.Fatal("default service config failed to be applied after 1s")
+	}
+}
+
+func testDefaultServiceConfigWhenResolverDoesNotReturnServiceConfig(t *testing.T, r resolver.Resolver, addr string, js string) {
+	cc, err := Dial(addr, WithInsecure(), WithDefaultServiceConfig(js))
+	if err != nil {
+		t.Fatalf("Dial(%s, _) = _, %v, want _, <nil>", addr, err)
+	}
+	defer cc.Close()
+	r.(*manual.Resolver).NewAddress([]resolver.Address{{Addr: addr}})
+	if !verifyWaitForReadyEqualsTrue(cc) {
 		t.Fatal("default service config failed to be applied after 1s")
 	}
 }
diff --git a/service_config.go b/service_config.go
index 982a3bf21e65..526fb55a3d5c 100644
--- a/service_config.go
+++ b/service_config.go
@@ -239,9 +239,6 @@ type jsonSC struct {
 }
 
 func parseServiceConfig(js string) (ServiceConfig, error) {
-	if len(js) == 0 {
-		return ServiceConfig{}, fmt.Errorf("no JSON service config provided")
-	}
 	var rsc jsonSC
 	err := json.Unmarshal([]byte(js), &rsc)
 	if err != nil {
@@ -370,3 +367,9 @@ func getMaxSize(mcMax, doptMax *int, defaultVal int) *int {
 func newInt(b int) *int {
 	return &b
 }
+
+// ValidateServiceConfig validates the input service config json string and returns the error.
+func ValidateServiceConfig(js string) error {
+	_, err := parseServiceConfig(js)
+	return err
+}