xds/client: hold authority mutex before making a new authority by menghanl · Pull Request #5331 · grpc/grpc-go

xds/client: hold authority mutex before making a new authority by menghanl · Pull Request #5331 · grpc/grpc-go

fixes #5328

Caller must hold the mutex before calling newAuthority.

RELEASE NOTES:

xds/client: fix a potential concurrent map read/write in load reporting

		// load reporting stream.
		func (c clientImpl) ReportLoad(server bootstrap.ServerConfig) (*load.Store, func()) {
		c.authorityMu.Lock()
		a, err := c.newAuthority(server)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we name the function newAuthorityLocked to better indicate you should have the lock when calling it? I see it's in the doc comments but those obviously weren't sufficient in this case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Rename this and several other methods.

		return store, func() {
		cancelF()
		c.unrefAuthority(a)
		c.unrefAuthorityLocked(a)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cleanup function always invoked with the lock held?

		func (c *clientImpl) WatchListener(serviceName string, cb func(xdsresource.ListenerUpdate, error)) (cancel func()) {
		n := xdsresource.ParseName(serviceName)
		a, unref, err := c.findAuthority(n)
		a, unref, err := c.findAuthorityLocked(n)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lock isn't held? And below?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, the other two actually says must not hold mu.....

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Geez, who wrote this code? 😉

github-actions bot locked as resolved and limited conversation to collaborators