Confusing for NewClient in 1.63

[SamYuan1990]

Please see the FAQ in our main README.md before submitting your issue.

NewClient

grpc-go/clientconn.go

Line 124 in c31cec3

// The DialOptions returned by WithBlock, WithTimeout, and

but

grpc-go/dialoptions.go

Line 392 in c31cec3

func WithTimeout(d time.Duration) DialOption {

// WithTimeout returns a DialOption that configures a timeout for dialing a
// ClientConn initially. This is valid if and only if WithBlock() is present.
//
// Deprecated: use DialContext instead of Dial and context.WithTimeout
// instead.  Will be supported throughout 1.x.

hence, which means in 1.63 we Deprecated timeout support?

as I found this issue by lint Hyperledger-TWGC/tape#384

if I change to the NewClient following unit test case

grpc-go/clientconn_test.go

Line 390 in c31cec3

func (s) TestWithTimeout(t *testing.T) {

I will use another Deprecated function as WithTimeout?

Any suggestion?

[SamYuan1990]

Further more,

grpc-go/clientconn.go

Lines 126 to 290 in c31cec3

	func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) {
	cc := &ClientConn{
	target: target,
	conns: make(map[*addrConn]struct{}),
	dopts: defaultDialOptions(),
	}
	cc.retryThrottler.Store((*retryThrottler)(nil))
	cc.safeConfigSelector.UpdateConfigSelector(&defaultConfigSelector{nil})
	cc.ctx, cc.cancel = context.WithCancel(context.Background())
	// Apply dial options.
	disableGlobalOpts := false
	for _, opt := range opts {
	if _, ok := opt.(*disableGlobalDialOptions); ok {
	disableGlobalOpts = true
	break
	}
	}
	if !disableGlobalOpts {
	for _, opt := range globalDialOptions {
	opt.apply(&cc.dopts)
	}
	}
	for _, opt := range opts {
	opt.apply(&cc.dopts)
	}
	chainUnaryClientInterceptors(cc)
	chainStreamClientInterceptors(cc)
	if err := cc.validateTransportCredentials(); err != nil {
	return nil, err
	}
	if cc.dopts.defaultServiceConfigRawJSON != nil {
	scpr := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON)
	if scpr.Err != nil {
	return nil, fmt.Errorf("%s: %v", invalidDefaultServiceConfigErrPrefix, scpr.Err)
	}
	cc.dopts.defaultServiceConfig, _ = scpr.Config.(*ServiceConfig)
	}
	cc.mkp = cc.dopts.copts.KeepaliveParams
	// Register ClientConn with channelz.
	cc.channelzRegistration(target)
	// TODO: Ideally it should be impossible to error from this function after
	// channelz registration. This will require removing some channelz logs
	// from the following functions that can error. Errors can be returned to
	// the user, and successful logs can be emitted here, after the checks have
	// passed and channelz is subsequently registered.
	// Determine the resolver to use.
	if err := cc.parseTargetAndFindResolver(); err != nil {
	channelz.RemoveEntry(cc.channelz.ID)
	return nil, err
	}
	if err = cc.determineAuthority(); err != nil {
	channelz.RemoveEntry(cc.channelz.ID)
	return nil, err
	}
	cc.csMgr = newConnectivityStateManager(cc.ctx, cc.channelz)
	cc.pickerWrapper = newPickerWrapper(cc.dopts.copts.StatsHandlers)
	cc.initIdleStateLocked() // Safe to call without the lock, since nothing else has a reference to cc.
	cc.idlenessMgr = idle.NewManager((*idler)(cc), cc.dopts.idleTimeout)
	return cc, nil
	}
	// Dial calls DialContext(context.Background(), target, opts...).
	//
	// Deprecated: use NewClient instead. Will be supported throughout 1.x.
	func Dial(target string, opts ...DialOption) (*ClientConn, error) {
	return DialContext(context.Background(), target, opts...)
	}
	// DialContext calls NewClient and then exits idle mode. If WithBlock(true) is
	// used, it calls Connect and WaitForStateChange until either the context
	// expires or the state of the ClientConn is Ready.
	//
	// One subtle difference between NewClient and Dial and DialContext is that the
	// former uses "dns" as the default name resolver, while the latter use
	// "passthrough" for backward compatibility. This distinction should not matter
	// to most users, but could matter to legacy users that specify a custom dialer
	// and expect it to receive the target string directly.
	//
	// Deprecated: use NewClient instead. Will be supported throughout 1.x.
	func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) {
	// At the end of this method, we kick the channel out of idle, rather than
	// waiting for the first rpc.
	opts = append([]DialOption{withDefaultScheme("passthrough")}, opts...)
	cc, err := NewClient(target, opts...)
	if err != nil {
	return nil, err
	}
	// We start the channel off in idle mode, but kick it out of idle now,
	// instead of waiting for the first RPC. This is the legacy behavior of
	// Dial.
	defer func() {
	if err != nil {
	cc.Close()
	}
	}()
	// This creates the name resolver, load balancer, etc.
	if err := cc.idlenessMgr.ExitIdleMode(); err != nil {
	return nil, err
	}
	// Return now for non-blocking dials.
	if !cc.dopts.block {
	return cc, nil
	}
	if cc.dopts.timeout > 0 {
	var cancel context.CancelFunc
	ctx, cancel = context.WithTimeout(ctx, cc.dopts.timeout)
	defer cancel()
	}
	defer func() {
	select {
	case <-ctx.Done():
	switch {
	case ctx.Err() == err:
	conn = nil
	case err == nil || !cc.dopts.returnLastError:
	conn, err = nil, ctx.Err()
	default:
	conn, err = nil, fmt.Errorf("%v: %v", ctx.Err(), err)
	}
	default:
	}
	}()
	// A blocking dial blocks until the clientConn is ready.
	for {
	s := cc.GetState()
	if s == connectivity.Idle {
	cc.Connect()
	}
	if s == connectivity.Ready {
	return cc, nil
	} else if cc.dopts.copts.FailOnNonTempDialError && s == connectivity.TransientFailure {
	if err = cc.connectionError(); err != nil {
	terr, ok := err.(interface {
	Temporary() bool
	})
	if ok && !terr.Temporary() {
	return nil, err
	}
	}
	}
	if !cc.WaitForStateChange(ctx, s) {
	// ctx got timeout or canceled.
	if err = cc.connectionError(); err != nil && cc.dopts.returnLastError {
	return nil, err
	}
	return nil, ctx.Err()
	}
	}
	}

as NewClient is going to replace DialContext,
the DialContext invokes Newclient and handle timeout out of NewClient... but from the code comments, it seems NewClient should handle timeout. But in fact it just handle idleTimeout?

[dfawley]

Setting a timeout when creating a client is an anti-pattern, and is not possible in any other gRPC implementations.

See https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#dialing-in-grpc for more info.

from the code comments, it seems NewClient should handle timeout

From which comments specifically? I'd like to cleanup the documentation as much as possible, so please let me know any places where you think it could be improved.

[SamYuan1990]

Setting a timeout when creating a client is an anti-pattern, and is not possible in any other gRPC implementations.

See https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#dialing-in-grpc for more info.

from the code comments, it seems NewClient should handle timeout

From which comments specifically? I'd like to cleanup the documentation as much as possible, so please let me know any places where you think it could be improved.

@dfawley , I suppose you are not get my point. I am not query on anti-pattern, or say if we apply the lint checking to current code. I am not sure if anti-pattern still works as for example WithTimeout is marked as Deprecated and the impl https://github.com/grpc/grpc-go/blob/master/clientconn.go#L244-L262 for timeout feature seems not anti-pattern ?

here are my steps, please go through them:

1. the lint reports https://github.com/grpc/grpc-go/blob/master/clientconn.go#L200C4-L200C14 as Deprecated hence I go to NewClient https://github.com/grpc/grpc-go/blob/master/clientconn.go#L126 as here.
2. at the NewClient, where https://github.com/grpc/grpc-go/blob/master/clientconn.go#L124-L125 seems I can use WithTimeout
3. but ...

   grpc-go/dialoptions.go

   Line 390 in c31cec3

   // Deprecated: use DialContext instead of Dial and context.WithTimeout

   WithTimeout is another Deprecated code

so...
if I follow the NewClient hint, I will use WithTimeout as a Deprecated function then the lint fails again.
or
back to the 1st as following here https://github.com/grpc/grpc-go/blob/master/clientconn.go#L215 try to just use NewClient instead of DialContext which I found the timeout handle at https://github.com/grpc/grpc-go/blob/master/clientconn.go#L244-L262 which means the NewClient does not equals with DialContext and I can't replace it.
As the NewClient at https://github.com/grpc/grpc-go/blob/master/clientconn.go#L194 seems just handle cc.dopts.idleTimeout but not cc.dopts.Timeout, does it supports timeout ? if yes, why https://github.com/grpc/grpc-go/blob/master/clientconn.go#L244-L262 impls timeout by itself?

step further to learn from test case, https://github.com/grpc/grpc-go/blob/master/clientconn_test.go#L390 as https://github.com/grpc/grpc-go/blob/master/clientconn_test.go#L392 the WithTimeout is marked with Deprecated, the lint will fail, which not helpful.

[dfawley]

at the NewClient, where master/clientconn.go#L124-L125 seems I can use WithTimeout

I'm not sure why you think it says you can use WithTimeout. It explicitly says the opposite exactly where you quoted it:

The DialOptions returned by WithBlock, WithTimeout, and WithReturnConnectionError are ignored by this function.

Feel free to make your linter ignore these deprecated functions. We will not be removing them.

But your best bet is to read and follow https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#dialing-in-grpc and stop using both Dial and WithTimeout. It will give you more consistent behavior with other languages and ensure you have proper error handling around where RPCs are occurring.

If you have specific parts of the documentation that you can point to that need to be updated, I'd appreciate it. We can cover that in #7049.