client: Change connectivity state to CONNECTING when creating the name resolver (#8710)

Fixes #7686

#### Current Behavior
- When client exits IDLE and creates the name resolver, it stays in IDLE
until the connectivity state is set by the LB policy.
- When exiting IDLE mode (because of `Connect` being called or because
of an RPC), if name resolver creation fails, we stay in IDLE.

#### New Behavior
- When the client exits IDLE and creates the name resolver, it moves to
CONNECTING. Moving forward, the connectivity state will be set by the LB
policy.
- When exiting IDLE mode (because of `Connect` being called or because
of an RPC), we have already moved to CONNECTING (because of the previous
bullet point). If name resolver creation fails, we will move to
TRANSIENT_FAILURE and start the idle timer and move back to IDLE when
the timer fires

#### Implementation details:
- The client channel now treats resolver build errors encountered during
exiting IDLE identically to resolver errors received prior to valid
updates.
- `Build` uses a new unsafe API on the idleness manager to mark the
channel as exited IDLE.
- The idleness Manager invokes the channel's `ExitIdleMode` (which now
does not return an error) and updates internal state to reflect that it
is no longer in IDLE.
- `OnFinish` call options are now invoked even if stream creation fails
during an RPC. This fulfills the guarantee for these options and ensures
the idleness Manager’s `activeCallsCount` remains accurate.

RELEASE NOTES:
- client: Change connectivity state to CONNECTING when creating the name
resolver (as part of exiting IDLE).
- client: Change connectivity state to TRANSIENT_FAILURE if name
resolver creation fails (as part of exiting IDLE).
- client: Change connectivity state to IDLE after idle timeout expires
even when current state is TRANSIENT_FAILURE.
- client: Fix a bug that resulted in `OnFinish` call option not being
invoked for RPCs where stream creation failed.

Author: easwars

clientconn.go

@@ -262,9 +262,10 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
 	}()
 
 	// This creates the name resolver, load balancer, etc.
-	if err := cc.idlenessMgr.ExitIdleMode(); err != nil {
-		return nil, err
+	if err := cc.exitIdleMode(); err != nil {
+		return nil, fmt.Errorf("failed to exit idle mode: %w", err)
 	}
+	cc.idlenessMgr.UnsafeSetNotIdle()
 
 	// Return now for non-blocking dials.
 	if !cc.dopts.block {
@@ -332,7 +333,7 @@ func (cc *ClientConn) addTraceEvent(msg string) {
 			Severity: channelz.CtInfo,
 		}
 	}
-	channelz.AddTraceEvent(logger, cc.channelz, 0, ted)
+	channelz.AddTraceEvent(logger, cc.channelz, 1, ted)
 }
 
 type idler ClientConn
@@ -341,26 +342,41 @@ func (i *idler) EnterIdleMode() {
 	(*ClientConn)(i).enterIdleMode()
 }
 
-func (i *idler) ExitIdleMode() error {
-	return (*ClientConn)(i).exitIdleMode()
+func (i *idler) ExitIdleMode() {
+	// Ignore the error returned from this method, because from the perspective
+	// of the caller (idleness manager), the channel would have always moved out
+	// of IDLE by the time this method returns.
+	(*ClientConn)(i).exitIdleMode()
 }
 
 // exitIdleMode moves the channel out of idle mode by recreating the name
 // resolver and load balancer.  This should never be called directly; use
 // cc.idlenessMgr.ExitIdleMode instead.
-func (cc *ClientConn) exitIdleMode() (err error) {
+func (cc *ClientConn) exitIdleMode() error {
 	cc.mu.Lock()
 	if cc.conns == nil {
 		cc.mu.Unlock()
 		return errConnClosing
 	}
 	cc.mu.Unlock()
 
+	// Set state to CONNECTING before building the name resolver
+	// so the channel does not remain in IDLE.
+	cc.csMgr.updateState(connectivity.Connecting)
+
 	// This needs to be called without cc.mu because this builds a new resolver
 	// which might update state or report error inline, which would then need to
 	// acquire cc.mu.
 	if err := cc.resolverWrapper.start(); err != nil {
-		return err
+		// If resolver creation fails, treat it like an error reported by the
+		// resolver before any valid udpates. Set channel's state to
+		// TransientFailure, and set an erroring picker with the resolver build
+		// error, which will returned as part of any subsequent RPCs.
+		logger.Warningf("Failed to start resolver: %v", err)
+		cc.csMgr.updateState(connectivity.TransientFailure)
+		cc.mu.Lock()
+		cc.updateResolverStateAndUnlock(resolver.State{}, err)
+		return fmt.Errorf("failed to start resolver: %w", err)
 	}
 
 	cc.addTraceEvent("exiting idle mode")
@@ -681,10 +697,8 @@ func (cc *ClientConn) GetState() connectivity.State {
 // Notice: This API is EXPERIMENTAL and may be changed or removed in a later
 // release.
 func (cc *ClientConn) Connect() {
-	if err := cc.idlenessMgr.ExitIdleMode(); err != nil {
-		cc.addTraceEvent(err.Error())
-		return
-	}
+	cc.idlenessMgr.ExitIdleMode()
+
 	// If the ClientConn was not in idle mode, we need to call ExitIdle on the
 	// LB policy so that connections can be created.
 	cc.mu.Lock()
@@ -735,8 +749,8 @@ func init() {
 	internal.EnterIdleModeForTesting = func(cc *ClientConn) {
 		cc.idlenessMgr.EnterIdleModeForTesting()
 	}
-	internal.ExitIdleModeForTesting = func(cc *ClientConn) error {
-		return cc.idlenessMgr.ExitIdleMode()
+	internal.ExitIdleModeForTesting = func(cc *ClientConn) {
+		cc.idlenessMgr.ExitIdleMode()
 	}
 }
 

clientconn_parsed_target_test.go

@@ -205,28 +205,31 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) {
 }
 
 func (s) TestParsedTarget_Failure_WithoutCustomDialer_WithNewClient(t *testing.T) {
-	targets := []string{
-		"",
-		"unix://a/b/c",
-		"unix://authority",
-		"unix-abstract://authority/a/b/c",
-		"unix-abstract://authority",
+	tests := []struct {
+		target        string
+		wantErrSubstr string
+	}{
+
+		{target: "", wantErrSubstr: "invalid target address"},
+		{target: "unix://a/b/c", wantErrSubstr: "invalid (non-empty) authority"},
+		{target: "unix://authority", wantErrSubstr: "invalid (non-empty) authority"},
+		{target: "unix-abstract://authority/a/b/c", wantErrSubstr: "invalid (non-empty) authority"},
+		{target: "unix-abstract://authority", wantErrSubstr: "invalid (non-empty) authority"},
 	}
 
-	for _, target := range targets {
-		t.Run(target, func(t *testing.T) {
+	for _, test := range tests {
+		t.Run(test.target, func(t *testing.T) {
 			ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 			defer cancel()
-			cc, err := NewClient(target, WithTransportCredentials(insecure.NewCredentials()))
+			cc, err := NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()))
 			if err != nil {
-				t.Fatalf("NewClient(%q) failed: %v", target, err)
+				t.Fatalf("NewClient(%q) failed: %v", test, err)
 			}
 			defer cc.Close()
-			const wantErrSubstr = "failed to exit idle mode"
 			if _, err := cc.NewStream(ctx, &StreamDesc{}, "/my.service.v1.MyService/UnaryCall"); err == nil {
-				t.Fatalf("NewStream() succeeded with target = %q, cc.parsedTarget = %+v, expected to fail", target, cc.parsedTarget)
-			} else if !strings.Contains(err.Error(), wantErrSubstr) {
-				t.Fatalf("NewStream() with target = %q returned unexpected error: got %v, want substring %q", target, err, wantErrSubstr)
+				t.Fatalf("NewStream() succeeded with target = %q, cc.parsedTarget = %+v, expected to fail", test, cc.parsedTarget)
+			} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
+				t.Fatalf("NewStream() with target = %q returned unexpected error: got %v, want substring %q", test, err, test.wantErrSubstr)
 			}
 		})
 	}

dial_test.go

@@ -20,6 +20,7 @@ package grpc
 
 import (
 	"context"
+	"fmt"
 	"net"
 	"strings"
 	"testing"
@@ -312,3 +313,36 @@ func (s) TestResolverAddressesWithTypedNilAttribute(t *testing.T) {
 type stringerVal struct{ s string }
 
 func (s stringerVal) String() string { return s.s }
+
+const errResolverBuilderScheme = "test-resolver-build-failure"
+
+// errResolverBuilder is a resolver builder that returns an error from its Build
+// method.
+type errResolverBuilder struct {
+	err error
+}
+
+func (b *errResolverBuilder) Build(resolver.Target, resolver.ClientConn, resolver.BuildOptions) (resolver.Resolver, error) {
+	return nil, b.err
+}
+
+func (b *errResolverBuilder) Scheme() string {
+	return errResolverBuilderScheme
+}
+
+// Tests that Dial returns an error if the resolver builder returns an error
+// from its Build method.
+func (s) TestDial_ResolverBuilder_Error(t *testing.T) {
+	resolverErr := fmt.Errorf("resolver builder error")
+	dopts := []DialOption{
+		WithTransportCredentials(insecure.NewCredentials()),
+		WithResolvers(&errResolverBuilder{err: resolverErr}),
+	}
+	_, err := Dial(errResolverBuilderScheme+":///test.server", dopts...)
+	if err == nil {
+		t.Fatalf("Dial() succeeded when it should have failed")
+	}
+	if !strings.Contains(err.Error(), resolverErr.Error()) {
+		t.Fatalf("Dial() failed with error %v, want %v", err, resolverErr)
+	}
+}

internal/idle/idle.go

@@ -21,7 +21,6 @@
 package idle
 
 import (
-	"fmt"
 	"math"
 	"sync"
 	"sync/atomic"
@@ -33,15 +32,15 @@ var timeAfterFunc = func(d time.Duration, f func()) *time.Timer {
 	return time.AfterFunc(d, f)
 }
 
-// Enforcer is the functionality provided by grpc.ClientConn to enter
-// and exit from idle mode.
-type Enforcer interface {
-	ExitIdleMode() error
+// ClientConn is the functionality provided by grpc.ClientConn to enter and exit
+// from idle mode.
+type ClientConn interface {
+	ExitIdleMode()
 	EnterIdleMode()
 }
 
-// Manager implements idleness detection and calls the configured Enforcer to
-// enter/exit idle mode when appropriate.  Must be created by NewManager.
+// Manager implements idleness detection and calls the ClientConn to enter/exit
+// idle mode when appropriate. Must be created by NewManager.
 type Manager struct {
 	// State accessed atomically.
 	lastCallEndTime           int64 // Unix timestamp in nanos; time when the most recent RPC completed.
@@ -51,8 +50,8 @@ type Manager struct {
 
 	// Can be accessed without atomics or mutex since these are set at creation
 	// time and read-only after that.
-	enforcer Enforcer // Functionality provided by grpc.ClientConn.
-	timeout  time.Duration
+	cc      ClientConn // Functionality provided by grpc.ClientConn.
+	timeout time.Duration
 
 	// idleMu is used to guarantee mutual exclusion in two scenarios:
 	// - Opposing intentions:
@@ -72,9 +71,9 @@ type Manager struct {
 
 // NewManager creates a new idleness manager implementation for the
 // given idle timeout.  It begins in idle mode.
-func NewManager(enforcer Enforcer, timeout time.Duration) *Manager {
+func NewManager(cc ClientConn, timeout time.Duration) *Manager {
 	return &Manager{
-		enforcer:         enforcer,
+		cc:               cc,
 		timeout:          timeout,
 		actuallyIdle:     true,
 		activeCallsCount: -math.MaxInt32,
@@ -127,7 +126,7 @@ func (m *Manager) handleIdleTimeout() {
 
 	// Now that we've checked that there has been no activity, attempt to enter
 	// idle mode, which is very likely to succeed.
-	if m.tryEnterIdleMode() {
+	if m.tryEnterIdleMode(true) {
 		// Successfully entered idle mode. No timer needed until we exit idle.
 		return
 	}
@@ -142,10 +141,13 @@ func (m *Manager) handleIdleTimeout() {
 // that, it performs a last minute check to ensure that no new RPC has come in,
 // making the channel active.
 //
+// checkActivity controls if a check for RPC activity, since the last time the
+// idle_timeout fired, is made.
+
 // Return value indicates whether or not the channel moved to idle mode.
 //
 // Holds idleMu which ensures mutual exclusion with exitIdleMode.
-func (m *Manager) tryEnterIdleMode() bool {
+func (m *Manager) tryEnterIdleMode(checkActivity bool) bool {
 	// Setting the activeCallsCount to -math.MaxInt32 indicates to OnCallBegin()
 	// that the channel is either in idle mode or is trying to get there.
 	if !atomic.CompareAndSwapInt32(&m.activeCallsCount, 0, -math.MaxInt32) {
@@ -166,7 +168,7 @@ func (m *Manager) tryEnterIdleMode() bool {
 		atomic.AddInt32(&m.activeCallsCount, math.MaxInt32)
 		return false
 	}
-	if atomic.LoadInt32(&m.activeSinceLastTimerCheck) == 1 {
+	if checkActivity && atomic.LoadInt32(&m.activeSinceLastTimerCheck) == 1 {
 		// A very short RPC could have come in (and also finished) after we
 		// checked for calls count and activity in handleIdleTimeout(), but
 		// before the CAS operation. So, we need to check for activity again.
@@ -177,44 +179,37 @@ func (m *Manager) tryEnterIdleMode() bool {
 	// No new RPCs have come in since we set the active calls count value to
 	// -math.MaxInt32. And since we have the lock, it is safe to enter idle mode
 	// unconditionally now.
-	m.enforcer.EnterIdleMode()
+	m.cc.EnterIdleMode()
 	m.actuallyIdle = true
 	return true
 }
 
 // EnterIdleModeForTesting instructs the channel to enter idle mode.
 func (m *Manager) EnterIdleModeForTesting() {
-	m.tryEnterIdleMode()
+	m.tryEnterIdleMode(false)
 }
 
 // OnCallBegin is invoked at the start of every RPC.
-func (m *Manager) OnCallBegin() error {
+func (m *Manager) OnCallBegin() {
 	if m.isClosed() {
-		return nil
+		return
 	}
 
 	if atomic.AddInt32(&m.activeCallsCount, 1) > 0 {
 		// Channel is not idle now. Set the activity bit and allow the call.
 		atomic.StoreInt32(&m.activeSinceLastTimerCheck, 1)
-		return nil
+		return
 	}
 
 	// Channel is either in idle mode or is in the process of moving to idle
 	// mode. Attempt to exit idle mode to allow this RPC.
-	if err := m.ExitIdleMode(); err != nil {
-		// Undo the increment to calls count, and return an error causing the
-		// RPC to fail.
-		atomic.AddInt32(&m.activeCallsCount, -1)
-		return err
-	}
-
+	m.ExitIdleMode()
 	atomic.StoreInt32(&m.activeSinceLastTimerCheck, 1)
-	return nil
 }
 
-// ExitIdleMode instructs m to call the enforcer's ExitIdleMode and update m's
+// ExitIdleMode instructs m to call the ClientConn's ExitIdleMode and update its
 // internal state.
-func (m *Manager) ExitIdleMode() error {
+func (m *Manager) ExitIdleMode() {
 	// Holds idleMu which ensures mutual exclusion with tryEnterIdleMode.
 	m.idleMu.Lock()
 	defer m.idleMu.Unlock()
@@ -231,20 +226,34 @@ func (m *Manager) ExitIdleMode() error {
 		//   m.ExitIdleMode.
 		//
 		// In any case, there is nothing to do here.
-		return nil
+		return
 	}
 
-	if err := m.enforcer.ExitIdleMode(); err != nil {
-		return fmt.Errorf("failed to exit idle mode: %w", err)
-	}
+	m.cc.ExitIdleMode()
 
 	// Undo the idle entry process. This also respects any new RPC attempts.
 	atomic.AddInt32(&m.activeCallsCount, math.MaxInt32)
 	m.actuallyIdle = false
 
 	// Start a new timer to fire after the configured idle timeout.
 	m.resetIdleTimerLocked(m.timeout)
-	return nil
+}
+
+// UnsafeSetNotIdle instructs the Manager to update its internal state to
+// reflect the reality that the channel is no longer in IDLE mode.
+//
+// N.B. This method is intended only for internal use by the gRPC client
+// when it exits IDLE mode **manually** from `Dial`. The callsite must ensure:
+//   - The channel was **actually in IDLE mode** immediately prior to the call.
+//   - There is **no concurrent activity** that could cause the channel to exit
+//     IDLE mode *naturally* at the same time.
+func (m *Manager) UnsafeSetNotIdle() {
+	m.idleMu.Lock()
+	defer m.idleMu.Unlock()
+
+	atomic.AddInt32(&m.activeCallsCount, math.MaxInt32)
+	m.actuallyIdle = false
+	m.resetIdleTimerLocked(m.timeout)
 }
 
 // OnCallEnd is invoked at the end of every RPC.