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

Code before:

// NewPool creates a new xDS client pool with the given bootstrap config.
//
// If a nil bootstrap config is passed, the caller is expected to pass the
// config when creating the xDS client by using NewClientWithConfig instead of
// NewClient. If they specify a nil config and use NewClient, client creation
// will fail.
func NewPool(config *bootstrap.Config) *Pool {
	return &Pool{
		clients: make(map[string]*clientImpl),
		getConfiguration: func() (*bootstrap.Config, error) {
			return config, nil
		},
	}
}

// getBootstrapConfiguration returns the bootstrap configuration loaded from
// the environment variables.
func (p *Pool) getBootstrapConfiguration() (*bootstrap.Config, error) {
	p.mu.Lock()
	defer p.mu.Unlock()
	config, err := p.getConfiguration()
	if err != nil {
		return nil, fmt.Errorf("xds: failed to read xDS bootstrap config from env vars:  %v", err)
	}
	if config == nil {
		return nil, fmt.Errorf("failed to read xDS bootstrap config from env vars: bootstrap environment variables (%q or %q) not defined", envconfig.XDSBootstrapFileNameEnv, envconfig.XDSBootstrapFileContentEnv)
	}
	return config, nil
}

// NewClientWithConfig returns an xDS client with the given name from the pool.
// If the client doesn't already exist, it creates a new xDS client using the
// provided config and adds it to the pool. The provided config takes precedence
// over any config passed during pool creation. This should be used when a
// non-default config is required. If the provided config is nil, it attempts
// to load the configuration from the environment variables.
//
// The second return value represents a close function which the caller is
// expected to invoke once they are done using the client.  It is safe for the
// caller to invoke this close function multiple times.
func (p *Pool) NewClientWithConfig(name string, metricsRecorder estats.MetricsRecorder, config *bootstrap.Config) (XDSClient, func(), error) {
	if config == nil {
		var err error
		config, err = p.getBootstrapConfiguration()
		if err != nil {
			return nil, nil, err
		}
	}
	return p.newRefCounted(name, metricsRecorder, defaultWatchExpiryTimeout, config)
}

Review comment (refactor):
I don't see a need for `bootstrap.GetConfiguration` to return `(nil, nil)` if none of the env vars are set. It can instead return an error indicating that none of expected env vars were set. That will simplify the handling here. We wont have to handle the `nil, nil` case specifically.

Code after:
//
// If a nil bootstrap config is passed, the caller is expected to pass the
// config when creating the xDS client by using NewClientWithConfig instead of
// NewClient. If they specify a nil config and use NewClient, client creation
// will fail.
func NewPool(config *bootstrap.Config) *Pool {
	return &Pool{
		clients: make(map[string]*clientImpl),
		getConfiguration: func() (*bootstrap.Config, error) {
			if config == nil {
				return nil, fmt.Errorf("xds: bootstrap config cannot be nil")
			}
			return config, nil
		},
	}
}

// getBootstrapConfiguration returns the config specified at pool creation time.
func (p *Pool) getBootstrapConfiguration() (*bootstrap.Config, error) {
	p.mu.Lock()
	defer p.mu.Unlock()
	config, err := p.getConfiguration()
	if err != nil {
		return nil, fmt.Errorf("xds: failed to read xDS bootstrap config:  %v", err)
	}
	return config, nil
}

// getConfig returns the provided config if it is non-nil. Otherwise, it
// delegates to getBootstrapConfiguration to retrieve the configuration.
func (p *Pool) getConfig(config *bootstrap.Config) (*bootstrap.Config, error) {
	if config != nil {
		return config, nil
	}
	return p.getBootstrapConfiguration()
}

// NewClientWithConfig returns an xDS client with the given name from the pool.
// If the client doesn't already exist, it creates a new xDS client using the
// provided config and adds it to the pool. The provided config takes precedence
// over any config passed during pool creation. This should be used when a
// non-default config is required. If the provided config is nil, it attempts
// to load the configuration provided at pool creation time.
//
// The second return value represents a close function which the caller is
// expected to invoke once they are done using the client.  It is safe for the
// caller to invoke this close function multiple times.
func (p *Pool) NewClientWithConfig(name string, metricsRecorder estats.MetricsRecorder, config *bootstrap.Config) (XDSClient, func(), error) {
	config, err := p.getConfig(config)
	if err != nil {
		return nil, nil, err