client: add GRPC_GO_REQUIRE_HANDSHAKE options to control connection behavior

[dfawley]

#2406

Possible settings of this environment variable:

• "hybrid" (default; removed after the 1.17 release): do not wait for handshake before considering a connection ready, but wait before considering successful.

• "on" (default after the 1.17 release): wait for handshake before considering a connection ready/successful.

• "off": do not wait for handshake before considering a connection ready/successful.

This setting will be completely removed after the 1.18 release, and "on" will be the only supported behavior.

[menghanl]

"hybrid" (default; removed after the 1.17 release): wait for handshake before considering a connection ready, but wait before considering successful.

Did you miss a "not"?

[dfawley]

Did you miss a "not"?

Yes. I edited this text like 5 times, so it doesn't surprise me that I killed it at some point.

[menghanl]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @dfawley, @jadekler, and @menghanl)

---

clientconn.go, line 1151 at r2 (raw file):

		prefaceMu.Lock()
		clientPrefaceWrote = true
		if serverPrefaceReceived || (!ac.dopts.waitForHandshake && ac.cc.reqHandshake == envconfig.RequireHandshakeOff) {

To make this a bit clearer, convert ac.dopts.waitForHandshake into ac.cc.reqHandshake so we don't need to check both?

// In Dial()
if dopts.waitForHandshake {
    cc.reqHandshake = envconfig.RequireHandshakeOn
}

---

clientconn.go, line 1169 at r2 (raw file):

				return nil
			}
		} else if ac.cc.reqHandshake == envconfig.RequireHandshakeHybrid {

if wait_for_handshake is on ...
else if hybird ...
// else (off)

The else (off) is not handled (which is fine because we don't do anything).
But it means we don't need prefaceTimer, and also we don't need the preface handler onPrefaceReceipt.

[menghanl]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @dfawley, @jadekler, and @menghanl)

---

clientconn_test.go, line 150 at r2 (raw file):

}

func TestDialWaitsForServerSettings(t *testing.T) {

This is to test with WithWaitForHandshake() dial option, env variable doesn't matter, right?
Add a comment for the test?

[dfawley]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @menghanl and @jadekler)

---

clientconn.go, line 1151 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

To make this a bit clearer, convert ac.dopts.waitForHandshake into ac.cc.reqHandshake so we don't need to check both?

// In Dial()
if dopts.waitForHandshake {
    cc.reqHandshake = envconfig.RequireHandshakeOn
}

One better: make the dopt an envconfig.RequireHandshakeSetting and share everything there.

---

clientconn.go, line 1169 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

if wait_for_handshake is on ...
else if hybird ...
// else (off)

The else (off) is not handled (which is fine because we don't do anything).
But it means we don't need prefaceTimer, and also we don't need the preface handler onPrefaceReceipt.

You are correct, but I'd rather not clutter code for a minor optimization when it will be deleted in 6 more weeks.

---

clientconn_test.go, line 150 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

This is to test with WithWaitForHandshake() dial option, env variable doesn't matter, right?
Add a comment for the test?

Correct. Done.

[jeanbza]

Should we deprecate WithWaitForHandshake, and/or add a note discussing the change in behavior in 1.17?

ready_connection_one

Hahahaha.

[dfawley]

Thanks for the review. Diffs updated.

[jeanbza]

(my small questions are resolved)

[NeoyeElf]

I have a problem here...

I used grpc-client(v1.18.0) to send request grpc-server(v1.16.0), it turned out that request hang up (or exceed deadline when set timeout). After I have checked the change log, I found this feat: client: add GRPC_GO_REQUIRE_HANDSHAKE options to control connection behavior . With my doubt, I set the env variables "GRPC_GO_REQUIRE_HANDSHAKE=off" then it worked.

here is my questions:
Is this the only way to make it work without downgrade google.golang.org/grpc to 1.16 for my grpc-client?

Thanks...