transport: remove RequireHandshakeHybrid support (#2529)

This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult https://github.com/soheilhy/cmux/issues/64.

Also, several tests have been updated to take this into consideration by
sending settings frames.

clientconn.go

@@ -1019,7 +1019,19 @@ func (ac *addrConn) resetTransport() {
 			reconnect := grpcsync.NewEvent()
 			prefaceReceived := make(chan struct{})
 			newTr, err := ac.createTransport(addr, copts, connectDeadline, reconnect, prefaceReceived)
-			if err == nil {
+			if err != nil {
+				ac.cc.blockingpicker.updateConnectionError(err)
+				hcancel()
+				if err == errConnClosing {
+					return
+				}
+
+				if tryNextAddrFromStart.HasFired() {
+					break addrLoop
+				}
+				continue
+			}
+
 			ac.mu.Lock()
 			ac.curAddr = addr
 			ac.transport = newTr
@@ -1047,57 +1059,10 @@ func (ac *addrConn) resetTransport() {
 				ac.updateConnectivityState(connectivity.Ready)
 				ac.mu.Unlock()
 			}
-			} else {
-				hcancel()
-				if err == errConnClosing {
-					return
-				}
-
-				if tryNextAddrFromStart.HasFired() {
-					break addrLoop
-				}
-				continue
-			}
-
-			ac.mu.Lock()
-			reqHandshake := ac.dopts.reqHandshake
-			ac.mu.Unlock()
 
 			<-reconnect.Done()
 			hcancel()
 
-			if reqHandshake == envconfig.RequireHandshakeHybrid {
-				// In RequireHandshakeHybrid mode, we must check to see whether
-				// server preface has arrived yet to decide whether to start
-				// reconnecting at the top of the list (server preface received)
-				// or continue with the next addr in the list as if the
-				// connection were not successful (server preface not received).
-				select {
-				case <-prefaceReceived:
-					// We received a server preface - huzzah! We consider this
-					// a success and restart from the top of the addr list.
-					ac.mu.Lock()
-					ac.backoffIdx = 0
-					ac.mu.Unlock()
-					break addrLoop
-				default:
-					// Despite having set state to READY, in hybrid mode we
-					// consider this a failure and continue connecting at the
-					// next addr in the list.
-					ac.mu.Lock()
-					if ac.state == connectivity.Shutdown {
-						ac.mu.Unlock()
-						return
-					}
-
-					ac.updateConnectivityState(connectivity.TransientFailure)
-					ac.mu.Unlock()
-
-					if tryNextAddrFromStart.HasFired() {
-						break addrLoop
-					}
-				}
-			} else {
 			// In RequireHandshakeOn mode, we would have already waited for
 			// the server preface, so we consider this a success and restart
 			// from the top of the addr list. In RequireHandshakeOff mode,
@@ -1109,7 +1074,6 @@ func (ac *addrConn) resetTransport() {
 			ac.mu.Unlock()
 			break addrLoop
 		}
-		}
 
 		// After exhausting all addresses, or after need to reconnect after a
 		// READY, the addrConn enters TRANSIENT_FAILURE.
@@ -1154,8 +1118,6 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne
 		Authority: ac.cc.authority,
 	}
 
-	prefaceTimer := time.NewTimer(time.Until(connectDeadline))
-
 	onGoAway := func(r transport.GoAwayReason) {
 		ac.mu.Lock()
 		ac.adjustParams(r)
@@ -1165,13 +1127,11 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne
 
 	onClose := func() {
 		close(onCloseCalled)
-		prefaceTimer.Stop()
 		reconnect.Fire()
 	}
 
 	onPrefaceReceipt := func() {
 		close(prefaceReceived)
-		prefaceTimer.Stop()
 	}
 
 	connectCtx, cancel := context.WithDeadline(ac.ctx, connectDeadline)
@@ -1181,38 +1141,8 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne
 	}
 
 	newTr, err := transport.NewClientTransport(connectCtx, ac.cc.ctx, target, copts, onPrefaceReceipt, onGoAway, onClose)
-
-	if err == nil {
-		if ac.dopts.reqHandshake == envconfig.RequireHandshakeOn {
-			select {
-			case <-prefaceTimer.C:
-				// We didn't get the preface in time.
-				newTr.Close()
-				err = errors.New("timed out waiting for server handshake")
-			case <-prefaceReceived:
-				// We got the preface - huzzah! things are good.
-			case <-onCloseCalled:
-				// The transport has already closed - noop.
-				return nil, errors.New("connection closed")
-			}
-		} else if ac.dopts.reqHandshake == envconfig.RequireHandshakeHybrid {
-			go func() {
-				select {
-				case <-prefaceTimer.C:
-					// We didn't get the preface in time.
-					newTr.Close()
-				case <-prefaceReceived:
-					// We got the preface just in the nick of time - huzzah!
-				case <-onCloseCalled:
-					// The transport has already closed - noop.
-				}
-			}()
-		}
-	}
-
 	if err != nil {
 		// newTr is either nil, or closed.
-		ac.cc.blockingpicker.updateConnectionError(err)
 		ac.mu.Lock()
 		if ac.state == connectivity.Shutdown {
 			// ac.tearDown(...) has been invoked.
@@ -1225,6 +1155,22 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne
 		return nil, err
 	}
 
+	if ac.dopts.reqHandshake == envconfig.RequireHandshakeOn {
+		select {
+		case <-time.After(connectDeadline.Sub(time.Now())):
+			// We didn't get the preface in time.
+			newTr.Close()
+			grpclog.Warningf("grpc: addrConn.createTransport failed to connect to %v: didn't receive server preface in time. Reconnecting...", addr)
+			return nil, errors.New("timed out waiting for server handshake")
+		case <-prefaceReceived:
+			// We got the preface - huzzah! things are good.
+		case <-onCloseCalled:
+			// The transport has already closed - noop.
+			return nil, errors.New("connection closed")
+			// TODO(deklerk) this should bail on ac.ctx.Done(). Add a test and fix.
+		}
+	}
+
 	// Now there is a viable transport to be use, so set ac.transport to reflect the new viable transport.
 	ac.mu.Lock()
 	if ac.state == connectivity.Shutdown {

clientconn_test.go

@@ -121,15 +121,6 @@ func (s) TestDialWithMultipleBackendsNotSendingServerPreface(t *testing.T) {
 var allReqHSSettings = []envconfig.RequireHandshakeSetting{
 	envconfig.RequireHandshakeOff,
 	envconfig.RequireHandshakeOn,
-	envconfig.RequireHandshakeHybrid,
-}
-var reqNoHSSettings = []envconfig.RequireHandshakeSetting{
-	envconfig.RequireHandshakeOff,
-	envconfig.RequireHandshakeHybrid,
-}
-var reqHSBeforeSuccess = []envconfig.RequireHandshakeSetting{
-	envconfig.RequireHandshakeOn,
-	envconfig.RequireHandshakeHybrid,
 }
 
 func (s) TestDialWaitsForServerSettings(t *testing.T) {
@@ -332,10 +323,8 @@ func (s) TestDialDoesNotWaitForServerSettings(t *testing.T) {
 	// Restore current setting after test.
 	old := envconfig.RequireHandshake
 	defer func() { envconfig.RequireHandshake = old }()
+	envconfig.RequireHandshake = envconfig.RequireHandshakeOff
 
-	// Test with "off" and "hybrid".
-	for _, setting := range reqNoHSSettings {
-		envconfig.RequireHandshake = setting
 	lis, err := net.Listen("tcp", "localhost:0")
 	if err != nil {
 		t.Fatalf("Error while listening. Err: %v", err)
@@ -358,7 +347,6 @@ func (s) TestDialDoesNotWaitForServerSettings(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 	client, err := DialContext(ctx, lis.Addr().String(), WithInsecure(), WithBlock())
-
 	if err != nil {
 		t.Fatalf("DialContext returned err =%v; want nil", err)
 	}
@@ -368,14 +356,13 @@ func (s) TestDialDoesNotWaitForServerSettings(t *testing.T) {
 		t.Fatalf("client.GetState() = %v; want connectivity.Ready", state)
 	}
 	close(dialDone)
-		<-done
-	}
 }
 
 func (s) TestCloseConnectionWhenServerPrefaceNotReceived(t *testing.T) {
 	// Restore current setting after test.
 	old := envconfig.RequireHandshake
 	defer func() { envconfig.RequireHandshake = old }()
+	envconfig.RequireHandshake = envconfig.RequireHandshakeOn
 
 	// 1. Client connects to a server that doesn't send preface.
 	// 2. After minConnectTimeout(500 ms here), client disconnects and retries.
@@ -384,10 +371,6 @@ func (s) TestCloseConnectionWhenServerPrefaceNotReceived(t *testing.T) {
 	cleanup := setMinConnectTimeout(time.Millisecond * 500)
 	defer cleanup()
 
-	// Test with "on" and "hybrid".
-	for _, setting := range reqHSBeforeSuccess {
-		envconfig.RequireHandshake = setting
-
 	lis, err := net.Listen("tcp", "localhost:0")
 	if err != nil {
 		t.Fatalf("Error while listening. Err: %v", err)
@@ -458,7 +441,6 @@ func (s) TestCloseConnectionWhenServerPrefaceNotReceived(t *testing.T) {
 	client.Close()
 	<-done
 }
-}
 
 func (s) TestBackoffWhenNoServerPrefaceReceived(t *testing.T) {
 	lis, err := net.Listen("tcp", "localhost:0")

internal/envconfig/envconfig.go

@@ -34,13 +34,9 @@ const (
 type RequireHandshakeSetting int
 
 const (
-	// RequireHandshakeHybrid (default, deprecated) indicates to not wait for
+	// RequireHandshakeOn indicates to wait for handshake before considering a
-	// handshake before considering a connection ready, but wait before
+	// connection ready/successful.
-	// considering successful.
+	RequireHandshakeOn RequireHandshakeSetting = iota
-	RequireHandshakeHybrid RequireHandshakeSetting = iota
-	// RequireHandshakeOn (default after the 1.17 release) indicates to wait
-	// for handshake before considering a connection ready/successful.
-	RequireHandshakeOn
 	// RequireHandshakeOff indicates to not wait for handshake before
 	// considering a connection ready/successful.
 	RequireHandshakeOff
@@ -53,7 +49,7 @@ var (
 	// environment variable.
 	//
 	// Will be removed after the 1.18 release.
-	RequireHandshake RequireHandshakeSetting
+	RequireHandshake = RequireHandshakeOn
 )
 
 func init() {
@@ -64,8 +60,5 @@ func init() {
 		RequireHandshake = RequireHandshakeOn
 	case "off":
 		RequireHandshake = RequireHandshakeOff
-	case "hybrid":
-		// Will be removed after the 1.17 release.
-		RequireHandshake = RequireHandshakeHybrid
 	}
 }

test/gracefulstop_test.go

@@ -27,8 +27,8 @@ import (
 	"testing"
 	"time"
 
+	"golang.org/x/net/http2"
 	"google.golang.org/grpc"
-
 	"google.golang.org/grpc/internal/envconfig"
 	testpb "google.golang.org/grpc/test/grpc_testing"
 )
@@ -52,6 +52,13 @@ func (d *delayListener) Accept() (net.Conn, error) {
 	default:
 		close(d.acceptCalled)
 		conn, err := d.Listener.Accept()
+		if err != nil {
+			return nil, err
+		}
+		framer := http2.NewFramer(conn, conn)
+		if err = framer.WriteSettings(http2.Setting{}); err != nil {
+			return nil, err
+		}
 		// Allow closing of listener only after accept.
 		// Note: Dial can return successfully, yet Accept
 		// might now have finished.
@@ -107,10 +114,14 @@ func (d *delayConn) Read(b []byte) (n int, err error) {
 }
 
 func (s) TestGracefulStop(t *testing.T) {
-	// Set default behavior and restore current setting after test.
+	// We need to turn off RequireHandshake because if it were on, it would
+	// block forever waiting to read the handshake, and the delayConn would
+	// never let it (the delay is intended to block until later in the test).
+	//
+	// Restore current setting after test.
 	old := envconfig.RequireHandshake
-	envconfig.RequireHandshake = envconfig.RequireHandshakeOff
 	defer func() { envconfig.RequireHandshake = old }()
+	envconfig.RequireHandshake = envconfig.RequireHandshakeOff
 
 	// This test ensures GracefulStop cannot race and break RPCs on new
 	// connections created after GracefulStop was called but before

vet.sh

@@ -119,5 +119,6 @@ google.golang.org/grpc/stats/stats_test.go:SA1019
 google.golang.org/grpc/test/channelz_test.go:SA1019
 google.golang.org/grpc/test/end2end_test.go:SA1019
 google.golang.org/grpc/test/healthcheck_test.go:SA1019
+google.golang.org/grpc/clientconn.go:S1024
 ' ./...
 misspell -error .