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

Code before:
	for _, a := range client.bootstrapConfig.Authorities() {
		for _, s := range a.XDSServers {
			for _, f := range s.Cleanups() {
				f()
			}
		}
	}
	p.mu.Unlock()

	// This attempts to close the transport to the management server and could
	// theoretically call back into the xdsclient package again and deadlock.
	// Hence, this needs to be called without holding the lock.
	client.Close()

	xdsClientImplCloseHook(name)
}

// newRefCounted creates a new reference counted xDS client implementation for
// name, if one does not exist already. If an xDS client for the given name
// exists, it gets a reference to it and returns it.
func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder, watchExpiryTimeout time.Duration, config *bootstrap.Config) (*clientImpl, func(), error) {
	p.mu.Lock()
	defer p.mu.Unlock()

	if config == nil {
		return nil, nil, fmt.Errorf("xds: bootstrap config cannot be nil")
	}

	if c := p.clients[name]; c != nil {
		c.incrRef()
		return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
	}

	c, err := newClientImpl(config, metricsRecorder, name, watchExpiryTimeout)
	if err != nil {
		return nil, nil, err
	}
	if logger.V(2) {
		c.logger.Infof("Created client with name %q and bootstrap configuration:\n %s", name, config)
	}
	p.clients[name] = c
	xdsClientImplCreateHook(name)

	logger.Infof("xDS node ID: %s", config.Node().GetId())
	return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
}


Review comment (suggestion):
I thought the whole point was to ensure that this is never called with a `nil` config and therefore not handle the `nil` case here at all. You could mention this in the docstring as well.

Code after:
	for _, a := range client.bootstrapConfig.Authorities() {
		for _, s := range a.XDSServers {
			for _, f := range s.Cleanups() {
				f()
			}
		}
	}
	p.mu.Unlock()

	// This attempts to close the transport to the management server and could
	// theoretically call back into the xdsclient package again and deadlock.
	// Hence, this needs to be called without holding the lock.
	client.Close()

	xdsClientImplCloseHook(name)
}

// newRefCounted creates a new reference counted xDS client implementation for
// name, if one does not exist already. If an xDS client for the given name
// exists, it gets a reference to it and returns it.
//
// The config should not be nil.
func (p *Pool) newRefCounted(name string, metricsRecorder estats.MetricsRecorder, watchExpiryTimeout time.Duration, config *bootstrap.Config) (*clientImpl, func(), error) {
	p.mu.Lock()
	defer p.mu.Unlock()

	if c := p.clients[name]; c != nil {
		c.incrRef()
		return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
	}

	c, err := newClientImpl(config, metricsRecorder, name, watchExpiryTimeout)
	if err != nil {
		return nil, nil, err
	}
	if logger.V(2) {
		c.logger.Infof("Created client with name %q and bootstrap configuration:\n %s", name, config)
	}
	p.clients[name] = c
	xdsClientImplCreateHook(name)

	logger.Infof("xDS node ID: %s", config.Node().GetId())
	return c, sync.OnceFunc(func() { p.clientRefCountedClose(name) }), nil
}