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.

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

Fixes #1272: fix map concurrent writes when rendering html templates in ui #1280

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

lx223
Copy link
Contributor

@lx223 lx223 commented Jun 26, 2019

Changes

  • use template.Funcs to overwrite prefix rendering function to avoid concurrent map writes

Verification

Run go run github.com/improbable-eng/thanos/cmd/thanos query
And then concurrently HTTP GET "http://0.0.0.0:10902/graph" 1000 times

Before change:
It panics with concurrent map writes.
After change:
No longer panics.

@lx223 lx223 changed the title iss-1272: fix concurrent writes when rendering html templates in ui Fixes #1272: fix concurrent writes when rendering html templates in ui Jun 26, 2019
@lx223 lx223 changed the title Fixes #1272: fix concurrent writes when rendering html templates in ui Fixes #1272: fix map concurrent writes when rendering html templates in ui Jun 26, 2019
@povilasv
Copy link
Member

This makes sense, we were writing to map concurrently. Thanks for fixing this!

@povilasv povilasv merged commit dbbeeb5 into thanos-io:master Jun 27, 2019
@lx223 lx223 deleted the iss-1272 branch June 27, 2019 13:11
@bwplotka
Copy link
Member

That was quick @lx223 ! (:

@bwplotka
Copy link
Member

Since we probably copied this part from Prometheus I would check Prometheus codebase as well if they have same bug 🤔

@bwplotka
Copy link
Member

Also we are missing CHANGELOG entry (: But it will be part of release shephard items to do.

@lx223
Copy link
Contributor Author

lx223 commented Jun 27, 2019

Ah, sorry. Should have added the entry in CHANGELOG. I'll put up another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants