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

Code before:
var (
	// The following functions are no-ops in the actual code, but can be
	// overridden in tests to give them visibility into certain events.
	xdsClientImplCreateHook = func(string) {}
	xdsClientImplCloseHook  = func(string) {}

	defaultStreamBackoffFunc = backoff.DefaultExponential.Backoff
)

func clientRefCountedClose(name string) {
	clientsMu.Lock()
	client, ok := clients[name]
	if !ok {
		logger.Errorf("Attempt to close a non-existent xDS client with name %s", name)
		clientsMu.Unlock()
		return
	}
	if client.decrRef() != 0 {
		clientsMu.Unlock()
		return
	}
	delete(clients, name)
	clientsMu.Unlock()

	// This attempts to close the transport to the management server and could
	// take a while to complete. So, call it without holding the lock.
	client.clientImpl.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 newRefCounted(name string, config *bootstrap.Config, watchExpiryTimeout time.Duration, streamBackoff func(int) time.Duration) (XDSClient, func(), error) {
	clientsMu.Lock()
	defer clientsMu.Unlock()

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

	// Create the new client implementation.
	c, err := newClientImpl(config, watchExpiryTimeout, streamBackoff)
	if err != nil {
		return nil, nil, err
	}
	c.logger.Infof("Created client with name %q and bootstrap configuration:\n %s", name, config)
	client := &clientRefCounted{clientImpl: c, refCount: 1}
	clients[name] = client

Review comment (bug):
I would rewrite this a little.  The reason to do it without the lock is not because it will "take a while" (because that implies it's purely for performance), but because the things it does could theoretically call into the `xdsclient` package again and deadlock.

Code after:
var (
	// The following functions are no-ops in the actual code, but can be
	// overridden in tests to give them visibility into certain events.
	xdsClientImplCreateHook = func(string) {}
	xdsClientImplCloseHook  = func(string) {}

	defaultStreamBackoffFunc = backoff.DefaultExponential.Backoff
)

func clientRefCountedClose(name string) {
	clientsMu.Lock()
	client, ok := clients[name]
	if !ok {
		logger.Errorf("Attempt to close a non-existent xDS client with name %s", name)
		clientsMu.Unlock()
		return
	}
	if client.decrRef() != 0 {
		clientsMu.Unlock()
		return
	}
	delete(clients, name)
	clientsMu.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.clientImpl.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 newRefCounted(name string, config *bootstrap.Config, watchExpiryTimeout time.Duration, streamBackoff func(int) time.Duration) (XDSClient, func(), error) {
	clientsMu.Lock()
	defer clientsMu.Unlock()

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

	// Create the new client implementation.
	c, err := newClientImpl(config, watchExpiryTimeout, streamBackoff)
	if err != nil {
		return nil, nil, err
	}
	c.logger.Infof("Created client with name %q and bootstrap configuration:\n %s", name, config)
	client := &clientRefCounted{clientImpl: c, refCount: 1}