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

Panic: assignment to entry in nil map #422

Closed
epsniff opened this issue Apr 15, 2015 · 9 comments · Fixed by #423
Closed

Panic: assignment to entry in nil map #422

epsniff opened this issue Apr 15, 2015 · 9 comments · Fixed by #423
Labels
client

Comments

@epsniff
Copy link
Contributor

epsniff commented Apr 15, 2015

Hi all,

I'm still digging in (updates to come) but we are seeing this panic from the sarama client:

panic: assignment to entry in nil map

goroutine 3857 [running]:
github.com/Shopify/sarama.(*client).registerBroker(0xc21e3a3f80, 0xc242620230)
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:367 +0xac
github.com/Shopify/sarama.(*client).updateMetadata(0xc21e3a3f80, 0xc234acde30, 0x0, 0x0, 0x0, 0x0, 0x0)
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:611 +0x127
github.com/Shopify/sarama.(*client).tryRefreshMetadata(0xc21e3a3f80, 0x0, 0x0, 0x0, 0x2, 0x0, 0x0)
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:563 +0x2d2
github.com/Shopify/sarama.(*client).tryRefreshMetadata(0xc21e3a3f80, 0x0, 0x0, 0x0, 0x3, 0x0, 0x0)
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:595 +0xb86
github.com/Shopify/sarama.(*client).RefreshMetadata(0xc21e3a3f80, 0x0, 0x0, 0x0, 0x0, 0x0)
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:301 +0x161
github.com/Shopify/sarama.(*client).backgroundMetadataUpdater(0xc21e3a3f80)
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:541 +0x10a
github.com/Shopify/sarama.*client.(github.com/Shopify/sarama.backgroundMetadataUpdater)·fm()
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:135 +0x27
github.com/Shopify/sarama.withRecover(0xc21db856d0)
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/utils.go:42 +0x3a
created by github.com/Shopify/sarama.NewClient
    /home/mdmarek/go/root/src/github.com/Shopify/sarama/client.go:135 +0x794
@wvanbergen
Copy link
Contributor

Can you run it while capturing sarama.Logger output?

@eapache
Copy link
Contributor

eapache commented Apr 15, 2015

Hmm, based on the backtrace alone, if we close the client while the background metadata updater is running there might be a race that looks something like this...

@epsniff
Copy link
Contributor Author

epsniff commented Apr 15, 2015

@eapache thats a good possibility. The client is running in a Metafora task, and those tasks can get moved between nodes in our cluster a number of times per day. Moving the tasks closes the client.

@epsniff
Copy link
Contributor Author

epsniff commented Apr 15, 2015

@wvanbergen Ill turn on logging, but it could take a couple of days before we hit it again :)

@epsniff
Copy link
Contributor Author

epsniff commented Apr 15, 2015

@eapache - I can see in our logs that we closed the client a few seconds before the panic.

@epsniff
Copy link
Contributor Author

epsniff commented Apr 15, 2015

Our broker list is static, so in the short-term I'm going to just disable to background updater by setting the RefreshFrequency to zero. https://github.com/Shopify/sarama/blob/9b048b0f558f2e7f420b11688d669aaa22d77d6a/config.go#L136 as a workaround.

@wvanbergen
Copy link
Contributor

Looks like we need to properly stop the background metadata updater before we can continue the shutdown process.

@epsniff FYI: we always turn on sarama logging even in production. In normal operation, you will only see a single log line every 10 minutes (the background updater), but when shit hits the fan the information is invaluable.

@eapache
Copy link
Contributor

eapache commented Apr 15, 2015

I have a unit test that reproduces this stack trace. Fix incoming shortly.

@epsniff
Copy link
Contributor Author

epsniff commented Apr 15, 2015

Thanks to both of you! Talk about opensource working! I'd like to see an enterprise product turn around a fix that fast. And I'll have sarama logging on in the future :).

eapache added a commit that referenced this issue Apr 15, 2015
@eapache
client: stop the background thread before closing
9c715b9 
Otherwise, in the rare case where a background metadata refresh coincided
exactly with a call to `Client.Close()` there was a race where we might have
potentially tried to write to a nil map. Also add a test for this.

Fixes #422.
@eapache eapache mentioned this issue Apr 15, 2015
Merged
eapache added a commit that referenced this issue Apr 15, 2015
@eapache
client: stop the background thread before closing
dd6ba39 
Otherwise, in the rare case where a background metadata refresh coincided
exactly with a call to `Client.Close()` there was a race where we might have
potentially tried to write to a nil map. Also add a test for this.

Fixes #422.
@eapache eapache mentioned this issue Apr 25, 2015
Closed
@handsomewwu handsomewwu mentioned this issue Apr 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
client
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

client: stop the background thread before closing IBM/sarama
3 participants
@wvanbergen @eapache @epsniff