fix registering broker on closed client

[clitetailor]

When i try to close sarama client i got the same error as #1531

panic: assignment to entry in nil map

As i understand this is a simple check-then-act race condition. Did i handle error correctly?!

[bai]

Thanks for contributing. Could you please sign CLA (see link in GitHub Checks list right below this comment)?

[dnwe]

@clitetailor thanks for your PR. The changes look reasonable, essentially checking again (after taking the write lock) that brokers is still non-nil before proceeding. It would be helpful if you could also add the panic stacktrace you were seeing into the description of the issue.

I wasn't sure why you'd chosen to remove the up-front read-locking if client.Closed() { return } check in the updateMetadata func, but left it in-place in the RefreshCoordinator func? It's also a change in behaviour as updateMetadata was previously returning false, nil when client.brokers was nil at entrypoint. Is that change intentional?

[clitetailor]

Hi @dnwe, thanks for your reply. Sorry cause i don't hold the code so i can't give you the stacktrace until next week but i can tell that the stacktrace is the same as described in #1531.

I wasn't sure why you'd chosen to remove the up-front read-locking if client.Closed() { return } check in the updateMetadata func, but left it in-place in the RefreshCoordinator func?

I remove the up-front check because i feel it's quite redundant to lock and check twice on the same condition.

I don't remove it in RefreshCoordinator to avoid futher requests and cause i'm not sure whether removing client.Closed() before calling client.getConsumerData() is safe either.

It's also a change in behaviour as updateMetadata was previously returning false, nil when client.brokers was nil at entrypoint.

I see that updateMetadata is called inside RefreshMetadata via tryRefreshMetadata and RefreshMetadata also returns ErrClosedClient when the client has already been closed so i do the same. However, i would revert those changes if it is not the expected behavior.

What do you think? Do you have any suggestion?

[clitetailor]

@dnwe FYI, here is my stacktrace:

panic: assignment to entry in nil map

goroutine 194 [running]:
github.com/Shopify/sarama.(*client).registerBroker(0xc00188c090, 0xc0014ba700)
	path-to-project-folder/vendor/github.com/Shopify/sarama/client.go:533 +0x443
github.com/Shopify/sarama.(*client).updateMetadata(0xc00188c090, 0xc00087c050, 0xc00087c000, 0x0, 0x0, 0x0)
	path-to-project-folder/vendor/github.com/Shopify/sarama/client.go:844 +0x136
github.com/Shopify/sarama.(*client).tryRefreshMetadata(0xc00188c090, 0xc0002c0860, 0x1, 0x1, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0)
	path-to-project-folder/vendor/github.com/Shopify/sarama/client.go:789 +0xd46
github.com/Shopify/sarama.(*client).RefreshMetadata(0xc00188c090, 0xc0002c0860, 0x1, 0x1, 0x0, 0x0)
	path-to-project-folder/vendor/github.com/Shopify/sarama/client.go:442 +0x2f5
github.com/Shopify/sarama.(*consumerGroup).topicToPartitionNumbers(0xc0015f03f0, 0xc0002c0860, 0x1, 0x1, 0x0, 0x0, 0x0)
	path-to-project-folder/vendor/github.com/Shopify/sarama/consumer_group.go:472 +0x97
github.com/Shopify/sarama.(*consumerGroup).loopCheckPartitionNumbers(0xc0015f03f0, 0xc0002c0860, 0x1, 0x1, 0xc001882b00)
	path-to-project-folder/vendor/github.com/Shopify/sarama/consumer_group.go:454 +0x1ed
created by github.com/Shopify/sarama.(*consumerGroup).Consume
	path-to-project-folder/vendor/github.com/Shopify/sarama/consumer_group.go:178 +0x428

Debugger finished with exit code 0

[clitetailor]

It seems like the problem has been fixed already. I will reopen if any furthur problem encountered.

Thanks for creating a great library!