client: backoff before reconnecting if an HTTP2 server preface was not received #1648

MakMukhi · 2017-11-03T21:00:58Z

dfawley · 2017-11-07T23:05:13Z

clientconn.go

@@ -114,6 +115,16 @@ func UseCompressor(name string) CallOption {
 	})
 }

+// WithWaitForServerSettings makes the Dial wait until the client receives initial settings (server preface)


How about:

With...Settings blocks until the initial settings frame is received from the server before assigning RPCs to the connection.

Also, how about WithWaitForHandshake? I think I'll be renaming the ServerOption to HandshakeTimeout to match Java/C.

dfawley · 2017-11-07T23:13:00Z

clientconn.go

 	// ready is closed and becomes nil when a new transport is up or failed
 	// due to timeout.
 	ready     chan struct{}
 	transport transport.ClientTransport

 	// The reason this addrConn is torn down.
 	tearDownErr error
+
+	retries int


Please give this a more specific name; I'm implementing a feature called "retry" too. 😄

dfawley · 2017-11-07T23:15:45Z

clientconn.go

+	// backoffDeadline is the time until which resetTransport needs to
+	// wait before increasing retries count.
+	backoffDeadline time.Time
+	// connectDeadline is the time by which dial and tls handshake should


and also the server preface, right?

How about "connectDeadline is the time by which all connection negotiation must complete"?

dfawley · 2017-11-07T23:17:32Z

clientconn.go

+// The created transport must receive initial settings frame from the server.
+// In case that doesnt happen, transportMonitor will kill the newly created
+// transport after connectDeadline has expired.
+// In case there was an error on the tranpsort before the settings frame was


Good catch!

dfawley · 2017-11-07T23:18:07Z

clientconn.go

+// transport after connectDeadline has expired.
+// In case there was an error on the tranpsort before the settings frame was
+// received, resetTransport resumes connecting to backends after the one that
+// is previously connected to. In case end of the list is reached, resetTransport


dfawley · 2017-11-17T17:36:45Z

clientconn.go

+			newTransport, err := transport.NewClientTransport(connectCtx, ac.cc.ctx, sinfo, copts, func() {
+				close(done)
+				ac.mu.Lock()
+				if !ac.backoffDeadline.IsZero() {


It may be faster (and is simpler, code-wise) to just clear everything unconditionally.

dfawley · 2017-11-17T17:41:16Z

clientconn.go

 		ac.mu.Unlock()
 		// Block until we receive a goaway or an error occurs.
 		select {
 		case <-t.GoAway():
 		case <-t.Error():
+		case <-cdeadline:
+			timer.Stop()


The timer fired; stopping it is unnecessary.

dfawley · 2017-11-17T17:44:30Z

clientconn.go

+			timer = nil
+			// No server preface received until deadline.
+			// Kill the connection.
+			grpclog.Errorf("grpc: addrConn.transportMonitor didn't get server preface after waiting. Closing the new transport now.")


dfawley · 2017-11-17T17:45:31Z

resolver/manual/manual.go

+	bootstrapAddrs []resolver.Address
+}
+
+// BootstrapWithAddrs adds resolved addresses to the resolver so that


"InitialAddrs"?

dfawley · 2017-11-17T17:49:11Z

vet.sh

@@ -64,7 +64,7 @@ trap cleanup EXIT
 git ls-files "*.go" | xargs sed -i 's:"golang.org/x/net/context":"context":'
 set +o pipefail
 # TODO: Stop filtering pb.go files once golang/protobuf#214 is fixed.
-go tool vet -all . 2>&1 | grep -vF '.pb.go:' | tee /dev/stderr | (! read)
+go tool vet -all . 2>&1 | grep -vE 'cancel (function|var)' | grep -vF '.pb.go:' | tee /dev/stderr | (! read)


What are you filtering out with this?

After a successful dial we can't call cancel on the context used for it's dialing because of Go 1.6 issue. But Go 1.9 vet catches that and throws an error with a message that this grep captures.

We used to do this only for the file it was in, IIRC. Can you make this grep for 'clientconn.go:.*cancel ........ ?

dfawley · 2017-11-28T18:58:40Z

clientconn.go

+// It returns true if a connection was established.
+func (ac *addrConn) createTransport(connectRetryNum int, backoffDeadline, connectDeadline time.Time, addrs []resolver.Address, prevAddr resolver.Address, copts transport.ConnectOptions) (bool, error) {
+	startFrom := 0
+	for idx, addr := range addrs {


Maybe put this inside a if prevAddr != resolver.Address{} for clarity?

Could prevAddr be an int instead to avoid the need for this search? If the address list is changed during a connection attempt, which is pretty unlikely, we could just restart from the top of the list and reset the backoff - what do you think?

dfawley · 2017-11-29T16:59:57Z

clientconn.go

-					// ac.tearDown(...) has been invoked.
-					ac.mu.Unlock()
-					return errConnClosing
+				if ac.state != connectivity.Shutdown {


Should this be TransientFailure?

If the state was Shutdown we don't want to put it to TransientFailure

Makes sense. If we are here the only possibilities are Connecting or Shutdown.

dfawley · 2017-11-29T17:02:52Z

clientconn.go

+			case <-done:
+			case <-connectCtx.Done():
+				// Didn't receive server preface, must kill this new
+				// transport now.


Nit: Unwrap please.

dfawley · 2017-11-29T22:08:13Z

clientconn.go

+			case <-connectCtx.Done():
+				// Didn't receive server preface, must kill this new
+				// transport now.
+				grpclog.Warningf("grpc: addrConn.createTransport will close the newly established transport since no server preface was received.")


How about: grpc: addrConn.createTransport failed to receive server preface before deadline ?

dfawley · 2017-11-29T22:08:24Z

clientconn.go

+				// transport now.
+				grpclog.Warningf("grpc: addrConn.createTransport will close the newly established transport since no server preface was received.")
+				newTr.Close()
+				continue


Not return false?

dfawley · 2017-11-29T22:13:24Z

clientconn.go

 		ac.cc.handleSubConnStateChange(ac.acbw, ac.state)
+		if ac.transport != nil {


Delete these lines; ac.transport must be nil.

dfawley · 2017-11-29T22:41:28Z

clientconn_test.go

+	select {
+	case <-sent:
+	default:
+		t.Fatalf("Dial returned before server settings were sent")


Isn't this still a little racy? We write the settings immediately after closing sent. I think we should sleep for a little bit after closing sent, too.

Dial waits until the settings are sent and the channel is closed before they are.

OK, I see.. So it is still possible it returned before the settings were sent in this case, because we close sent before sending, but it's unlikely because of the sleep before sending.

dfawley · 2017-11-29T23:23:24Z

vet.sh

@@ -64,7 +64,7 @@ trap cleanup EXIT
 git ls-files "*.go" | xargs sed -i 's:"golang.org/x/net/context":"context":'
 set +o pipefail
 # TODO: Stop filtering pb.go files once golang/protobuf#214 is fixed.
-go tool vet -all . 2>&1 | grep -vF '.pb.go:' | tee /dev/stderr | (! read)
+go tool vet -all . 2>&1 | grep -vE 'cancel (function|var)' | grep -vF '.pb.go:' | tee /dev/stderr | (! read)


We used to do this only for the file it was in, IIRC. Can you make this grep for 'clientconn.go:.*cancel ........ ?

dfawley

1 nit and 1 minor cleanup, otherwise LGTM.

dfawley · 2017-11-30T23:31:45Z

clientconn.go

-					// ac.tearDown(...) has been invoked.
-					ac.mu.Unlock()
-					return errConnClosing
+				if ac.state != connectivity.Shutdown {


Makes sense. If we are here the only possibilities are Connecting or Shutdown.

dfawley · 2017-11-30T23:40:00Z

clientconn_test.go

+	select {
+	case <-sent:
+	default:
+		t.Fatalf("Dial returned before server settings were sent")


OK, I see.. So it is still possible it returned before the settings were sent in this case, because we close sent before sending, but it's unlikely because of the sleep before sending.

dfawley · 2017-11-30T23:42:06Z

clientconn.go

-	state connectivity.State
+	mu           sync.Mutex
+	curAddr      resolver.Address
+	reconnectIdx int // The index in adder list to start reconnecting from.


Nit: "addrs"

dfawley · 2017-11-30T23:50:34Z

clientconn.go

+		case <-cdeadline:
+			var isConnected bool
+			ac.mu.Lock()
+			if ac.backoffDeadline.IsZero() {


isConnected := ac.backoffDeadline.IsZero() // Client received server preface.

When security is disabled, not waiting for the HTTP/2 handshake can lead to DoS-style behavior. For details, see: grpc/grpc-go#954. This requirement will incur an extra half-RTT latency before the first RPC can be sent under plaintext, but this is negligible and unencrypted connections are rarer than secure ones. Under TLS, the server will effectively send its part of the HTTP/2 handshake along with its final TLS "server finished" message, which the client must wait for before transmitting any data securely. This means virtually no extra latency is incurred by this requirement. Go had attempted to separate "connection ready" with "connection successful" (Issue: grpc/grpc-go#1444 PR: grpc/grpc-go#1648). However, this is confusing to users and introduces an arbitrary distinction between these two events. It has led to several bugs in our reconnection logic (e.g.s grpc/grpc-go#2380, grpc/grpc-go#2391, grpc/grpc-go#2392), due to the complexity, and it makes custom transports (grpc/proposal#103) more difficult for users to implement. We are aware of some use cases (in particular, https://github.com/soheilhy/cmux) expecting the behavior of transmitting an RPC before the HTTP/2 handshake is completed. Before making behavior changes to implement this, we will reach out to our users to the best of our abilities.

dfawley changed the title ~~Client must backoff before retrying if an HTTP2 connection wasn't established( server preface wasn't received).~~ client: backoff before reconnecting if an HTTP2 server preface was not received Nov 3, 2017

dfawley self-requested a review November 6, 2017 22:07

dfawley assigned MakMukhi Nov 6, 2017

MakMukhi assigned dfawley and unassigned MakMukhi Nov 7, 2017

MakMukhi force-pushed the backoff_please branch 3 times, most recently from 9a60fe7 to 31151d2 Compare November 9, 2017 18:49

dfawley reviewed Nov 17, 2017

View reviewed changes

dfawley assigned MakMukhi and unassigned dfawley Nov 17, 2017

MakMukhi force-pushed the backoff_please branch from 4edbaca to 21b0f5e Compare November 28, 2017 00:01

MakMukhi assigned dfawley and unassigned MakMukhi Nov 28, 2017

MakMukhi force-pushed the backoff_please branch from 21b0f5e to f1f3cf6 Compare November 28, 2017 00:08

dfawley reviewed Nov 29, 2017

View reviewed changes

MakMukhi added 7 commits November 30, 2017 10:53

First commit.

c1530f6

Added rest of the features and test cases.

aef0bf7

Fix golang version related issues.

cbb7965

Struggles with vet.

69269cb

One more test.

0db18b1

Shame of misspelled words.

887c8db

Post-review update.

16769cf

dfawley mentioned this pull request Nov 30, 2017

Client connection phase should optionally wait for SETTINGS frame and set deadlines #1444

Closed

MakMukhi force-pushed the backoff_please branch 2 times, most recently from c77d7fc to 57890cb Compare November 30, 2017 22:54

Post-review update.

c8bfe9a

MakMukhi force-pushed the backoff_please branch from 57890cb to c8bfe9a Compare November 30, 2017 22:56

spellings!

593a808

dfawley approved these changes Dec 1, 2017

View reviewed changes

MakMukhi added 2 commits November 30, 2017 16:46

Fix racy test.

f8dda55

Post review update.

431db65

MakMukhi merged commit ddbb27e into grpc:master Dec 1, 2017

wladh mentioned this pull request Dec 29, 2017

testing: add tests covering races in server settings handshake #1772

Open

dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Jan 2, 2018

dfawley added this to the 1.9 Release milestone Jan 2, 2018

MakMukhi deleted the backoff_please branch May 4, 2018 01:53

This was referenced Oct 25, 2018

Add protocol handshake to 'READY' connectivity requirements grpc/grpc#17006

Merged

Implement new requirement of completing protocol handshaking before 'READY' #2406

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: backoff before reconnecting if an HTTP2 server preface was not received #1648

client: backoff before reconnecting if an HTTP2 server preface was not received #1648

MakMukhi commented Nov 3, 2017 •

edited

Loading

dfawley Nov 7, 2017

dfawley Nov 7, 2017

dfawley Nov 7, 2017

dfawley Nov 7, 2017

zasweq May 10, 2022

dfawley Nov 7, 2017

dfawley Nov 17, 2017

dfawley Nov 17, 2017

dfawley Nov 17, 2017

dfawley Nov 17, 2017

dfawley Nov 17, 2017

MakMukhi Nov 27, 2017

dfawley Nov 29, 2017

dfawley Nov 28, 2017

dfawley Nov 29, 2017

MakMukhi Nov 30, 2017

dfawley Nov 30, 2017

dfawley Nov 29, 2017

dfawley Nov 29, 2017

dfawley Nov 29, 2017

dfawley Nov 29, 2017

dfawley Nov 29, 2017

MakMukhi Nov 30, 2017

dfawley Nov 30, 2017

dfawley Nov 29, 2017

dfawley left a comment

dfawley Nov 30, 2017

dfawley Nov 30, 2017

dfawley Nov 30, 2017

dfawley Nov 30, 2017

		ac.cc.handleSubConnStateChange(ac.acbw, ac.state)
		if ac.transport != nil {

client: backoff before reconnecting if an HTTP2 server preface was not received #1648

client: backoff before reconnecting if an HTTP2 server preface was not received #1648

Conversation

MakMukhi commented Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MakMukhi commented Nov 3, 2017 •

edited

Loading