Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixed panic on calling updateMetadata on closed client #1531

Merged
merged 3 commits into from
Nov 12, 2019
Merged

fixed panic on calling updateMetadata on closed client #1531

merged 3 commits into from
Nov 12, 2019

Conversation

roblaszczak
Copy link
Contributor

@roblaszczak roblaszczak commented Nov 1, 2019 •
edited
Loading

Hello, after updateing sarama to v1.24.1 in https://github.com/ThreeDotsLabs/watermill-kafka our tests are failing because of assignment to entry in nil map:

[watermill] 2019/11/01 16:20:41.213125 subscriber.go:211: 	level=INFO  msg="Starting consuming" consumer_group=test kafka_consumer_uuid=jPK7fjGRZHsbSueM94mnqg provider=kafka subscriber_uuid=GZqfP4ixUbbvvoQYMHJcVF topic=topic_620ff2d2-616d-472e-a016-8329edb4738d 
[watermill] 2019/11/01 16:20:41.505179 subscriber.go:298: 	level=DEBUG msg="Consume stopped without any error" consumer_group=cg_9d09a750-cee9-402d-b6e9-5549781ef810 kafka_consumer_uuid=fsXW4sWL3Bs2XPoaHY5PXj provider=kafka subscriber_uuid=6PLBzrNsMkvZZFjPzWxa9F topic=topic_a1916770-8048-4928-ae01-3768f760d229 
[watermill] 2019/11/01 16:20:41.506553 subscriber.go:304: 	level=DEBUG msg="Group closed" consumer_group=cg_9d09a750-cee9-402d-b6e9-5549781ef810 kafka_consumer_uuid=fsXW4sWL3Bs2XPoaHY5PXj provider=kafka subscriber_uuid=6PLBzrNsMkvZZFjPzWxa9F topic=topic_a1916770-8048-4928-ae01-3768f760d229 
[watermill] 2019/11/01 16:20:41.506622 subscriber.go:307: 	level=INFO  msg="Consuming done" consumer_group=cg_9d09a750-cee9-402d-b6e9-5549781ef810 kafka_consumer_uuid=fsXW4sWL3Bs2XPoaHY5PXj provider=kafka subscriber_uuid=6PLBzrNsMkvZZFjPzWxa9F topic=topic_a1916770-8048-4928-ae01-3768f760d229 
[watermill] 2019/11/01 16:20:41.506718 subscriber.go:249: 	level=DEBUG msg="Client closed" consumer_group=cg_9d09a750-cee9-402d-b6e9-5549781ef810 kafka_consumer_uuid=fsXW4sWL3Bs2XPoaHY5PXj provider=kafka subscriber_uuid=6PLBzrNsMkvZZFjPzWxa9F topic=topic_a1916770-8048-4928-ae01-3768f760d229 
[watermill] 2019/11/01 16:20:41.506775 subscriber.go:172: 	level=DEBUG msg="consumeMessages stopped" consumer_group=cg_9d09a750-cee9-402d-b6e9-5549781ef810 kafka_consumer_uuid=fsXW4sWL3Bs2XPoaHY5PXj provider=kafka subscriber_uuid=6PLBzrNsMkvZZFjPzWxa9F topic=topic_a1916770-8048-4928-ae01-3768f760d229 
[watermill] 2019/11/01 16:20:41.506828 subscriber.go:181: 	level=DEBUG msg="Closing subscriber, no reconnect needed" consumer_group=cg_9d09a750-cee9-402d-b6e9-5549781ef810 kafka_consumer_uuid=fsXW4sWL3Bs2XPoaHY5PXj provider=kafka subscriber_uuid=6PLBzrNsMkvZZFjPzWxa9F topic=topic_a1916770-8048-4928-ae01-3768f760d229 
[watermill] 2019/11/01 16:20:41.506867 subscriber.go:417: 	level=DEBUG msg="Kafka subscriber closed" subscriber_uuid=6PLBzrNsMkvZZFjPzWxa9F 
panic: assignment to entry in nil map

goroutine 1939 [running]:
github.com/Shopify/sarama.(*client).registerBroker(0xc0015b6f30, 0xc004cce380)
	/home/robert/go/pkg/mod/github.com/!shopify/[email protected]/client.go:533 +0x2cb
github.com/Shopify/sarama.(*client).updateMetadata(0xc0015b6f30, 0xc00254a500, 0xc00254a500, 0x0, 0x0, 0x0)
	/home/robert/go/pkg/mod/github.com/!shopify/[email protected]/client.go:844 +0xde
github.com/Shopify/sarama.(*client).tryRefreshMetadata(0xc0015b6f30, 0xc00027e1e0, 0x1, 0x1, 0x3, 0x0, 0x0, 0x0, 0xd83b20, 0x1)
	/home/robert/go/pkg/mod/github.com/!shopify/[email protected]/client.go:789 +0x831
github.com/Shopify/sarama.(*client).RefreshMetadata(0xc0015b6f30, 0xc00027e1e0, 0x1, 0x1, 0x28, 0x7f9011ab2601)
	/home/robert/go/pkg/mod/github.com/!shopify/[email protected]/client.go:442 +0xb0
github.com/Shopify/sarama.(*consumerGroup).topicToPartitionNumbers(0xc000179560, 0xc00027e1e0, 0x1, 0x1, 0xc000036500, 0xc001b4d6b8, 0x4)
	/home/robert/go/pkg/mod/github.com/!shopify/[email protected]/consumer_group.go:472 +0x72
github.com/Shopify/sarama.(*consumerGroup).loopCheckPartitionNumbers(0xc000179560, 0xc00027e1e0, 0x1, 0x1, 0xc0015e3980)
	/home/robert/go/pkg/mod/github.com/!shopify/[email protected]/consumer_group.go:450 +0x105
created by github.com/Shopify/sarama.(*consumerGroup).Consume
	/home/robert/go/pkg/mod/github.com/!shopify/[email protected]/consumer_group.go:178 +0x236
FAIL	github.com/ThreeDotsLabs/watermill-kafka/v2/pkg/kafka	3.635s

It can be reproduced by running in https://github.com/ThreeDotsLabs/watermill-kafka on commit bdc2102750adb8d43389a8bd9d1621e645c787dc from fix-tests branch repository:

go get -u github.com/Shopify/[email protected]
docker-compose up -d
go test -parallel 20 ./... -short -race

@ghost ghost added cla-needed and removed cla-needed labels Nov 1, 2019
@roblaszczak
Copy link
Contributor Author

I added a couple more in 93f4001.

@roblaszczak
Copy link
Contributor Author

There was an error with the build (some timeout on starting Kafka). I restarted the build and it is green now :)

summon @bai

@bai
Copy link
Contributor

bai commented Nov 12, 2019

👋Thanks for contributing!

@bai bai merged commit afedeca into IBM:master Nov 12, 2019
@d1egoaz
Copy link
Contributor

d1egoaz commented Nov 14, 2019

I wonder if there is a way to add some test for this @roblaszczak ?

@roblaszczak
Copy link
Contributor Author

Probably it may be a good idea to add some tests around concurrent Close() and Close() during receiving messages (in https://github.com/ThreeDotsLabs/watermill we are creating and closing subscription at least once per test scenario, so we are calling it pretty often in some wired situations ;) ).

@roblaszczak
Copy link
Contributor Author

BTW @bai, any news on releasing it? It would be nice to be able to finally update Sarama version :)

@F21
Copy link

F21 commented Dec 17, 2019

Currently seeing this issue intermittently with 1.24.1. Can you guys tag a new release?

@roblaszczak roblaszczak mentioned this pull request Jan 2, 2020
Closed
dbudworth
dbudworth reviewed Jan 14, 2020
View reviewed changes
client.go
@@ -529,6 +532,11 @@ func (client *client) RefreshCoordinator(consumerGroup string) error {
// in the brokers map. It returns the broker that is registered, which may be the provided broker,
// or a previously registered Broker instance. You must hold the write lock before calling this function.
func (client *client) registerBroker(broker *Broker) {
if client.brokers == nil {
Copy link

Choose a reason for hiding this comment

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

this is a race condition, no?

It checks for client.brokers != nil
and then uses client.brokers

it could have been set to nil between the if client.brokers == nil and if client.brokers[broker.ID()] == nil

@joy4eg joy4eg mentioned this pull request Jan 14, 2020
Closed
@kimtree kimtree mentioned this pull request Jan 28, 2020
Closed
@clitetailor clitetailor mentioned this pull request Feb 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbudworth dbudworth dbudworth left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@roblaszczak @bai @d1egoaz @F21 @dbudworth