Repository: grpc/grpc-go
File: internal/xds/clients/xdsclient/xdsclient.go (Go)

Code before:
func New(config Config) (*XDSClient, error) {
	switch {
	case config.ResourceTypes == nil:
		return nil, errors.New("xdsclient: resource types map is nil")
	case config.TransportBuilder == nil:
		return nil, errors.New("xdsclient: transport builder is nil")
	case config.Authorities == nil && config.Servers == nil:
		return nil, errors.New("xdsclient: no servers or authorities specified")
	}

	client, err := newClient(&config, name)
	if err != nil {
		return nil, err
	}
	return client, nil
}

// SetWatchExpiryTimeoutForTesting override the default watch expiry timeout
// with provided timeout value and returns a function to reset the timeout to
// the original value.
//
// Note: This function should not be called concurrently and must be called
// before any RPCs to avoid race conditions.
func SetWatchExpiryTimeoutForTesting(watchExpiryTimeout time.Duration) func() {
	originalWatchTimeout := xdsclientinternal.WatchExpiryTimeout
	if watchExpiryTimeout == 0 {
		watchExpiryTimeout = defaultWatchExpiryTimeout
	}
	xdsclientinternal.WatchExpiryTimeout = watchExpiryTimeout
	return func() {
		xdsclientinternal.WatchExpiryTimeout = originalWatchTimeout
	}
}

// newClient returns a new XDSClient with the given config.
func newClient(config *Config, target string) (*XDSClient, error) {
	ctx, cancel := context.WithCancel(context.Background())
	c := &XDSClient{
		target:             target,
		done:               syncutil.NewEvent(),
		authorities:        make(map[string]*authority),
		config:             config,
		watchExpiryTimeout: xdsclientinternal.WatchExpiryTimeout,
		backoff:            xdsclientinternal.StreamBackoff,
		serializer:         syncutil.NewCallbackSerializer(ctx),
		serializerClose:    cancel,
		transportBuilder:   config.TransportBuilder,
		resourceTypes:      config.ResourceTypes,
		xdsActiveChannels:  make(map[ServerConfig]*channelState),
		metricsReporter:    config.MetricsReporter,
	}

Review comment (suggestion):
What I had in mind was to get rid of this function completely. Instead do the following:

- In `Pool.NewClientForTesting`, set the value of `WatchExpiryTimeout` defined in `internal/xds/clients/xdsclient/internal/internal.go`, and revert the value as part of the returned cancel function.
  - Now, if you cannot do that because the value of defined in an internal package and you don't have access to it from the package where `Pool.NewClientForTesting` is defined, please add a `SetWatchExpiryTimeoutForTesting` in the external xdsclient so that it is accessible from here. And in the docstring of that function, please specify that this method should be called before creating the xDS client.
- If you end up adding the above specified function, you can even unexport the `WatchExpiryTimeout` value as you now have a way to write to it through a function.
  
With the above approach:
- Existing tests would just have to set the `WatchExpiryTimeout` field inside of `OptionsForTesting` when calling `NewClientForTesting` and will not have to deal with resetting the value, as calling the returned cancel func would end up doing that.

Please let me know if this sounds OK to you. Thanks.

Code after:
	switch {
	case config.ResourceTypes == nil:
		return nil, errors.New("xdsclient: resource types map is nil")
	case config.TransportBuilder == nil:
		return nil, errors.New("xdsclient: transport builder is nil")
	case config.Authorities == nil && config.Servers == nil:
		return nil, errors.New("xdsclient: no servers or authorities specified")
	}
	if config.WatchExpiryTimeout == 0 {
		config.WatchExpiryTimeout = defaultWatchExpiryTimeout
	}
	client, err := newClient(&config, name)
	if err != nil {
		return nil, err
	}
	return client, nil
}

// newClient returns a new XDSClient with the given config.
func newClient(config *Config, target string) (*XDSClient, error) {
	ctx, cancel := context.WithCancel(context.Background())
	c := &XDSClient{
		target:             target,
		done:               syncutil.NewEvent(),
		authorities:        make(map[string]*authority),
		config:             config,
		watchExpiryTimeout: config.WatchExpiryTimeout,
		backoff:            xdsclientinternal.StreamBackoff,
		serializer:         syncutil.NewCallbackSerializer(ctx),
		serializerClose:    cancel,
		transportBuilder:   config.TransportBuilder,
		resourceTypes:      config.ResourceTypes,
		xdsActiveChannels:  make(map[ServerConfig]*channelState),
		metricsReporter:    config.MetricsReporter,
	}

	for name, cfg := range config.Authorities {
		// If server configs are specified in the authorities map, use that.
		// Else, use the top-level server configs.
		serverCfg := config.Servers
		if len(cfg.XDSServers) >= 1 {
			serverCfg = cfg.XDSServers
		}
		c.authorities[name] = newAuthority(authorityBuildOptions{
			serverConfigs:    serverCfg,
			name:             name,
			serializer:       c.serializer,
			getChannelForADS: c.getChannelForADS,
			logPrefix:        clientPrefix(c),
			target:           target,
			metricsReporter:  c.metricsReporter,