grpc: clean up doc strings and some code around Dial vs NewClient (#7029)

Author: dfawley

clientconn.go

@@ -101,11 +101,6 @@ const (
 	defaultReadBufSize  = 32 * 1024
 )
 
-// Dial creates a client connection to the given target.
-func Dial(target string, opts ...DialOption) (*ClientConn, error) {
-	return DialContext(context.Background(), target, opts...)
-}
-
 type defaultConfigSelector struct {
 	sc *ServiceConfig
 }
@@ -117,11 +112,22 @@ func (dcs *defaultConfigSelector) SelectConfig(rpcInfo iresolver.RPCInfo) (*ires
 	}, nil
 }
 
-func newClient(target, defaultScheme string, opts ...DialOption) (conn *ClientConn, err error) {
+// NewClient creates a new gRPC "channel" for the target URI provided.  No I/O
+// is performed.  Use of the ClientConn for RPCs will automatically cause it to
+// connect.  Connect may be used to manually create a connection, but for most
+// users this is unnecessary.
+//
+// The target name syntax is defined in
+// https://github.com/grpc/grpc/blob/master/doc/naming.md.  e.g. to use dns
+// resolver, a "dns:///" prefix should be applied to the target.
+//
+// The DialOptions returned by WithBlock, WithTimeout, and
+// WithReturnConnectionError are ignored by this function.
+func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) {
 	cc := &ClientConn{
 		target: target,
 		conns:  make(map[*addrConn]struct{}),
-		dopts:  defaultDialOptions(defaultScheme),
+		dopts:  defaultDialOptions(),
 		czData: new(channelzData),
 	}
 
@@ -190,45 +196,36 @@ func newClient(target, defaultScheme string, opts ...DialOption) (conn *ClientCo
 	return cc, nil
 }
 
-// NewClient returns a new client in idle mode.
-func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) {
-	return newClient(target, "dns", opts...)
+// Dial calls DialContext(context.Background(), target, opts...).
+//
+// Deprecated: use NewClient instead.  Will be supported throughout 1.x.
+func Dial(target string, opts ...DialOption) (*ClientConn, error) {
+	return DialContext(context.Background(), target, opts...)
 }
 
-// DialContext creates a client connection to the given target. By default, it's
-// a non-blocking dial (the function won't wait for connections to be
-// established, and connecting happens in the background). To make it a blocking
-// dial, use WithBlock() dial option.
+// DialContext calls NewClient and then exits idle mode.  If WithBlock(true) is
+// used, it calls Connect and WaitForStateChange until either the context
+// expires or the state of the ClientConn is Ready.
 //
-// In the non-blocking case, the ctx does not act against the connection. It
-// only controls the setup steps.
+// One subtle difference between NewClient and Dial and DialContext is that the
+// former uses "dns" as the default name resolver, while the latter use
+// "passthrough" for backward compatibility.  This distinction should not matter
+// to most users, but could matter to legacy users that specify a custom dialer
+// and expect it to receive the target string directly.
 //
-// In the blocking case, ctx can be used to cancel or expire the pending
-// connection. Once this function returns, the cancellation and expiration of
-// ctx will be noop. Users should call ClientConn.Close to terminate all the
-// pending operations after this function returns.
-//
-// The target name syntax is defined in
-// https://github.com/grpc/grpc/blob/master/doc/naming.md.
-// e.g. to use dns resolver, a "dns:///" prefix should be applied to the target.
+// Deprecated: use NewClient instead.  Will be supported throughout 1.x.
 func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) {
-	// At the end of this method, we kick the channel out of idle, rather than waiting for the first rpc.
-	cc, err := newClient(target, "passthrough", opts...)
+	// At the end of this method, we kick the channel out of idle, rather than
+	// waiting for the first rpc.
+	opts = append([]DialOption{withDefaultScheme("passthrough")}, opts...)
+	cc, err := NewClient(target, opts...)
 	if err != nil {
 		return nil, err
 	}
 
 	// We start the channel off in idle mode, but kick it out of idle now,
-	// instead of waiting for the first RPC. Other gRPC implementations do wait
-	// for the first RPC to kick the channel out of idle. But doing so would be
-	// a major behavior change for our users who are used to seeing the channel
-	// active after Dial.
-	//
-	// Taking this approach of kicking it out of idle at the end of this method
-	// allows us to share the code between channel creation and exiting idle
-	// mode. This will also make it easy for us to switch to starting the
-	// channel off in idle, i.e. by making newClient exported.
-
+	// instead of waiting for the first RPC.  This is the legacy behavior of
+	// Dial.
 	defer func() {
 		if err != nil {
 			cc.Close()
@@ -712,15 +709,15 @@ func init() {
 	}
 }
 
-func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) {
+func (cc *ClientConn) maybeApplyDefaultServiceConfig() {
 	if cc.sc != nil {
-		cc.applyServiceConfigAndBalancer(cc.sc, nil, addrs)
+		cc.applyServiceConfigAndBalancer(cc.sc, nil)
 		return
 	}
 	if cc.dopts.defaultServiceConfig != nil {
-		cc.applyServiceConfigAndBalancer(cc.dopts.defaultServiceConfig, &defaultConfigSelector{cc.dopts.defaultServiceConfig}, addrs)
+		cc.applyServiceConfigAndBalancer(cc.dopts.defaultServiceConfig, &defaultConfigSelector{cc.dopts.defaultServiceConfig})
 	} else {
-		cc.applyServiceConfigAndBalancer(emptyServiceConfig, &defaultConfigSelector{emptyServiceConfig}, addrs)
+		cc.applyServiceConfigAndBalancer(emptyServiceConfig, &defaultConfigSelector{emptyServiceConfig})
 	}
 }
 
@@ -738,7 +735,7 @@ func (cc *ClientConn) updateResolverStateAndUnlock(s resolver.State, err error)
 		// May need to apply the initial service config in case the resolver
 		// doesn't support service configs, or doesn't provide a service config
 		// with the new addresses.
-		cc.maybeApplyDefaultServiceConfig(nil)
+		cc.maybeApplyDefaultServiceConfig()
 
 		cc.balancerWrapper.resolverError(err)
 
@@ -750,9 +747,9 @@ func (cc *ClientConn) updateResolverStateAndUnlock(s resolver.State, err error)
 	var ret error
 	if cc.dopts.disableServiceConfig {
 		channelz.Infof(logger, cc.channelzID, "ignoring service config from resolver (%v) and applying the default because service config is disabled", s.ServiceConfig)
-		cc.maybeApplyDefaultServiceConfig(s.Addresses)
+		cc.maybeApplyDefaultServiceConfig()
 	} else if s.ServiceConfig == nil {
-		cc.maybeApplyDefaultServiceConfig(s.Addresses)
+		cc.maybeApplyDefaultServiceConfig()
 		// TODO: do we need to apply a failing LB policy if there is no
 		// default, per the error handling design?
 	} else {
@@ -765,7 +762,7 @@ func (cc *ClientConn) updateResolverStateAndUnlock(s resolver.State, err error)
 			} else {
 				configSelector = &defaultConfigSelector{sc}
 			}
-			cc.applyServiceConfigAndBalancer(sc, configSelector, s.Addresses)
+			cc.applyServiceConfigAndBalancer(sc, configSelector)
 		} else {
 			ret = balancer.ErrBadResolverState
 			if cc.sc == nil {
@@ -1072,7 +1069,7 @@ func (cc *ClientConn) getTransport(ctx context.Context, failfast bool, method st
 	})
 }
 
-func (cc *ClientConn) applyServiceConfigAndBalancer(sc *ServiceConfig, configSelector iresolver.ConfigSelector, addrs []resolver.Address) {
+func (cc *ClientConn) applyServiceConfigAndBalancer(sc *ServiceConfig, configSelector iresolver.ConfigSelector) {
 	if sc == nil {
 		// should never reach here.
 		return
@@ -1747,7 +1744,7 @@ func (cc *ClientConn) parseTargetAndFindResolver() error {
 	// scheme, except when a custom dialer is specified in which case, we should
 	// always use passthrough scheme. For either case, we need to respect any overridden
 	// global defaults set by the user.
-	defScheme := cc.dopts.defScheme
+	defScheme := cc.dopts.defaultScheme
 	if internal.UserSetDefaultScheme {
 		defScheme = resolver.GetDefaultScheme()
 	}

clientconn_parsed_target_test.go

@@ -33,91 +33,116 @@ import (
 	"google.golang.org/grpc/resolver"
 )
 
-func generateTarget(scheme string, target string) resolver.Target {
-	return resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", scheme, target))}
+func generateTarget(target string) resolver.Target {
+	return resolver.Target{URL: *testutils.MustParseURL(target)}
 }
 
-// This is here just in case another test calls the SetDefaultScheme method.
+// Resets the default scheme as though it was never set by the user.
 func resetInitialResolverState() {
 	resolver.SetDefaultScheme("passthrough")
 	internal.UserSetDefaultScheme = false
 }
 
+type testResolverForParser struct {
+	resolver.Resolver
+}
+
+func (testResolverForParser) Build(resolver.Target, resolver.ClientConn, resolver.BuildOptions) (resolver.Resolver, error) {
+	return testResolverForParser{}, nil
+}
+
+func (testResolverForParser) Close() {}
+
+func (testResolverForParser) Scheme() string {
+	return "testresolverforparser"
+}
+
+func init() { resolver.Register(testResolverForParser{}) }
+
 func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
-	resetInitialResolverState()
-	dialScheme := resolver.GetDefaultScheme()
-	newClientScheme := "dns"
 	tests := []struct {
 		target             string
 		wantDialParse      resolver.Target
 		wantNewClientParse resolver.Target
+		wantCustomParse    resolver.Target
 	}{
 		// No scheme is specified.
 		{
 			target:             "://a/b",
-			wantDialParse:      generateTarget(dialScheme, "://a/b"),
-			wantNewClientParse: generateTarget(newClientScheme, "://a/b"),
+			wantDialParse:      generateTarget("passthrough:///://a/b"),
+			wantNewClientParse: generateTarget("dns:///://a/b"),
+			wantCustomParse:    generateTarget("testresolverforparser:///://a/b"),
 		},
 		{
 			target:             "a//b",
-			wantDialParse:      generateTarget(dialScheme, "a//b"),
-			wantNewClientParse: generateTarget(newClientScheme, "a//b"),
+			wantDialParse:      generateTarget("passthrough:///a//b"),
+			wantNewClientParse: generateTarget("dns:///a//b"),
+			wantCustomParse:    generateTarget("testresolverforparser:///a//b"),
 		},
 
 		// An unregistered scheme is specified.
 		{
 			target:             "a:///",
-			wantDialParse:      generateTarget(dialScheme, "a:///"),
-			wantNewClientParse: generateTarget(newClientScheme, "a:///"),
+			wantDialParse:      generateTarget("passthrough:///a:///"),
+			wantNewClientParse: generateTarget("dns:///a:///"),
+			wantCustomParse:    generateTarget("testresolverforparser:///a:///"),
 		},
 		{
 			target:             "a:b",
-			wantDialParse:      generateTarget(dialScheme, "a:b"),
-			wantNewClientParse: generateTarget(newClientScheme, "a:b"),
+			wantDialParse:      generateTarget("passthrough:///a:b"),
+			wantNewClientParse: generateTarget("dns:///a:b"),
+			wantCustomParse:    generateTarget("testresolverforparser:///a:b"),
 		},
 
 		// A registered scheme is specified.
 		{
 			target:             "dns://a.server.com/google.com",
-			wantDialParse:      resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com")},
-			wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("dns://a.server.com/google.com")},
+			wantDialParse:      generateTarget("dns://a.server.com/google.com"),
+			wantNewClientParse: generateTarget("dns://a.server.com/google.com"),
+			wantCustomParse:    generateTarget("dns://a.server.com/google.com"),
 		},
 		{
 			target:             "unix-abstract:/ a///://::!@#$%25^&*()b",
-			wantDialParse:      resolver.Target{URL: *testutils.MustParseURL("unix-abstract:/ a///://::!@#$%25^&*()b")},
-			wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:/ a///://::!@#$%25^&*()b")},
+			wantDialParse:      generateTarget("unix-abstract:/ a///://::!@#$%25^&*()b"),
+			wantNewClientParse: generateTarget("unix-abstract:/ a///://::!@#$%25^&*()b"),
+			wantCustomParse:    generateTarget("unix-abstract:/ a///://::!@#$%25^&*()b"),
 		},
 		{
 			target:             "unix-abstract:passthrough:abc",
-			wantDialParse:      resolver.Target{URL: *testutils.MustParseURL("unix-abstract:passthrough:abc")},
-			wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("unix-abstract:passthrough:abc")},
+			wantDialParse:      generateTarget("unix-abstract:passthrough:abc"),
+			wantNewClientParse: generateTarget("unix-abstract:passthrough:abc"),
+			wantCustomParse:    generateTarget("unix-abstract:passthrough:abc"),
 		},
 		{
 			target:             "passthrough:///unix:///a/b/c",
-			wantDialParse:      resolver.Target{URL: *testutils.MustParseURL("passthrough:///unix:///a/b/c")},
-			wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("passthrough:///unix:///a/b/c")},
+			wantDialParse:      generateTarget("passthrough:///unix:///a/b/c"),
+			wantNewClientParse: generateTarget("passthrough:///unix:///a/b/c"),
+			wantCustomParse:    generateTarget("passthrough:///unix:///a/b/c"),
 		},
 
 		// Cases for `scheme:absolute-path`.
 		{
 			target:             "dns:/a/b/c",
-			wantDialParse:      resolver.Target{URL: *testutils.MustParseURL("dns:/a/b/c")},
-			wantNewClientParse: resolver.Target{URL: *testutils.MustParseURL("dns:/a/b/c")},
+			wantDialParse:      generateTarget("dns:/a/b/c"),
+			wantNewClientParse: generateTarget("dns:/a/b/c"),
+			wantCustomParse:    generateTarget("dns:/a/b/c"),
 		},
 		{
 			target:             "unregistered:/a/b/c",
-			wantDialParse:      generateTarget(dialScheme, "unregistered:/a/b/c"),
-			wantNewClientParse: generateTarget(newClientScheme, "unregistered:/a/b/c"),
+			wantDialParse:      generateTarget("passthrough:///unregistered:/a/b/c"),
+			wantNewClientParse: generateTarget("dns:///unregistered:/a/b/c"),
+			wantCustomParse:    generateTarget("testresolverforparser:///unregistered:/a/b/c"),
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.target, func(t *testing.T) {
+			resetInitialResolverState()
 			cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()))
 			if err != nil {
 				t.Fatalf("Dial(%q) failed: %v", test.target, err)
 			}
-			defer cc.Close()
+			cc.Close()
 
 			if !cmp.Equal(cc.parsedTarget, test.wantDialParse) {
 				t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantDialParse)
@@ -127,13 +152,36 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
 			if err != nil {
 				t.Fatalf("NewClient(%q) failed: %v", test.target, err)
 			}
-			defer cc.Close()
+			cc.Close()
 
 			if !cmp.Equal(cc.parsedTarget, test.wantNewClientParse) {
 				t.Errorf("cc.parsedTarget for newClient target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantNewClientParse)
 			}
+
+			resolver.SetDefaultScheme("testresolverforparser")
+			cc, err = Dial(test.target, WithTransportCredentials(insecure.NewCredentials()))
+			if err != nil {
+				t.Fatalf("Dial(%q) failed: %v", test.target, err)
+			}
+			cc.Close()
+
+			if !cmp.Equal(cc.parsedTarget, test.wantCustomParse) {
+				t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantDialParse)
+			}
+
+			cc, err = NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()))
+			if err != nil {
+				t.Fatalf("NewClient(%q) failed: %v", test.target, err)
+			}
+			cc.Close()
+
+			if !cmp.Equal(cc.parsedTarget, test.wantCustomParse) {
+				t.Errorf("cc.parsedTarget for newClient target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantNewClientParse)
+			}
+
 		})
 	}
+	resetInitialResolverState()
 }
 
 func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) {

dialoptions.go

@@ -79,7 +79,7 @@ type dialOptions struct {
 	resolvers                   []resolver.Builder
 	idleTimeout                 time.Duration
 	recvBufferPool              SharedBufferPool
-	defScheme                   string
+	defaultScheme               string
 }
 
 // DialOption configures how we set up the connection.
@@ -632,7 +632,7 @@ func withHealthCheckFunc(f internal.HealthChecker) DialOption {
 	})
 }
 
-func defaultDialOptions(defScheme string) dialOptions {
+func defaultDialOptions() dialOptions {
 	return dialOptions{
 		copts: transport.ConnectOptions{
 			ReadBufferSize:  defaultReadBufSize,
@@ -644,7 +644,7 @@ func defaultDialOptions(defScheme string) dialOptions {
 		healthCheckFunc: internal.HealthCheckFunc,
 		idleTimeout:     30 * time.Minute,
 		recvBufferPool:  nopBufferPool{},
-		defScheme:       defScheme,
+		defaultScheme:   "dns",
 	}
 }
 
@@ -659,6 +659,14 @@ func withMinConnectDeadline(f func() time.Duration) DialOption {
 	})
 }
 
+// withDefaultScheme is used to allow Dial to use "passthrough" as the default
+// name resolver, while NewClient uses "dns" otherwise.
+func withDefaultScheme(s string) DialOption {
+	return newFuncDialOption(func(o *dialOptions) {
+		o.defaultScheme = s
+	})
+}
+
 // WithResolvers allows a list of resolver implementations to be registered
 // locally with the ClientConn without needing to be globally registered via
 // resolver.Register.  They will be matched against the scheme used for the