clientconn: go idle if conn closed after preface received

[fuweid]

Fixes #5688

The select branch will be selected randomly, if there are several ready
branches. If the preface has been received and then connection is
closed, the createTransport might hit the connClosed.Done() branch.
Ideally, the subConn should go idle and reconnect.

RELEASE NOTES:

• client: fix race that could lead to an incorrect connection state if it was closed immediately after the server's HTTP/2 preface was received

[fuweid]

ping @dfawley ~

[fuweid]

Hi @zasweq would you mind to take a look on this? Thanks

We are hitting this issue if my understanding is correct ~

[dfawley]

Sorry, I was out of town for a few days; I can review this.

Close+reopen to make GA run the recently-added Go 1.19 tests.

[dfawley]

I believe in the event this case of the select occurs, onClose should be called, which will set our state to IDLE. Are you not seeing that?

(By the same logic, I think setting the state to IDLE in the case where the preface was received but connClose.HasFired() is true should also be unnecessary.)

[dfawley]

Ideally, the subConn should go idle and reconnect.

Maybe there's a misunderstanding as to the functionality here? gRPC will not reconnect automatically unless there is a new RPC or ClientConn.Connect() is called.

[fuweid]

Hi @dfawley thanks for the review.

In the onClose function, if the hcStarted == false, it will not set it IDLE. I am trying to add some sleep time to reproduce it in that issue #5688. Hope it can help~

if !hcStarted || hctx.Err() != nil {
			// We didn't start the health check or set the state to READY, so
			// no need to do anything else here.
			//
			// OR, we have already cancelled the health check context, meaning
			// we have already called onClose once for this transport.  In this
			// case it would be dangerous to clear the transport and update the
			// state, since there may be a new transport in this addrConn.
			return
		}

gRPC will not reconnect automatically unless there is a new RPC or ClientConn.Connect() is called.

For the unary call, the caller's goroutine is just waiting to get subConn. The gRPC client's background loop-goroutine will try to connect it if the subConn is idle.

grpc-go/balancer/base/balancer.go

Line 205 in fe59226

sc.Connect()

[fuweid]

It seems that the CI issue https://github.com/grpc/grpc-go/actions/runs/3269279382/jobs/5376604126 is not related to this change.

[dfawley]

Flaky test is #5631

[dfawley]

Aha, I see. I think this is fine, then. What we really need to do is make the preface receipt part of creating the transport, synchronously, to avoid all this extra complexity. But this fixes the small race that we have, so LGTM.

[fuweid]

reopen to rerun CI.

[easwars]

Thanks for your contribution @fuweid !!

[fuweid]

Thanks for your contribution @fuweid !!

Thanks for the review!