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

Fix race on measurementFields #6190

Merged
merged 2 commits into from
Apr 6, 2016
Merged

Fix race on measurementFields #6190

merged 2 commits into from
Apr 6, 2016

Conversation

jwilder
Copy link
Contributor

@jwilder jwilder commented Apr 1, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated

Both Shard and Engine had the same reference to the measurementField map,
but they each protected it with their own locks. This causes a race when
write and queries are occurring because writes can add new fields to the
map while queries are reading from it.

The fix moves the ownership to the Engine and provides protected accessors
to that Shard now users. For the most parts, the access on shard were old
dead code.

Fixing the measurementFields map race created a new race on the internal
fields map. This is now unexported and protected via MeasurementFields
exported funcs.

Fixes #6188


return nil
}

func (m *MeasurementFields) Field(name string) *Field {
m.mu.RLock()
defer m.mu.RUnlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

How often is this called? If it's a heavily-used function, maybe read, unlock, then return?

Both Shard and Engine had the same reference to the measurementField map,
but they each protected it with their own locks.  This causes a race when
write and queries are occurring because writes can add new fields to the
map while queries are reading from it.

The fix moves the ownership to the Engine and provides protected accessors
to that Shard now users.  For the most parts, the access on shard were old
dead code.

Fixing the measurementFields map race created a new race on the internal
fields map.  This is now unexported and protected via MeasurementFields
exported funcs.

Fixes #6188
Also remove some dead code that is no longer relevant with tsm.
@corylanou
Copy link
Contributor

+1

@joelegasse
Copy link
Contributor

LGTM

@joelegasse joelegasse merged commit 84f8dd7 into master Apr 6, 2016
@e-dard
Copy link
Contributor

e-dard commented Apr 6, 2016

Assigned(shardID uint64) is not reachable in this program, and nothing outside of meta.go refers to the shardIDs map on Series. Do we even need a shardIDs map on Series?

@syepes
Copy link

syepes commented Apr 6, 2016

@e-dard Thanks!

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.

Data race on measurementFields
5 participants