Skip to content

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

Merged
povilasv merged 1 commit intothanos-io:masterfrom
lx223:iss-1272
Jun 27, 2019
Merged

Fixes #1272: fix map concurrent writes when rendering html templates in ui#1280
povilasv merged 1 commit intothanos-io:masterfrom
lx223:iss-1272

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