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

tsm: cache: improve thread safety of Cache.Deduplicate (see #5699) #5701

Merged
merged 5 commits into from
Feb 25, 2016
Merged

tsm: cache: improve thread safety of Cache.Deduplicate (see #5699) #5701

merged 5 commits into from
Feb 25, 2016

Commits on Feb 25, 2016

  1. tsm: cache: add a test that demonstrates concurrent reads are safe 

    Signed-off-by: Jon Seymour <jon@wildducktheories.com>
    @jonseymour
    jonseymour committed Feb 25, 2016
    Configuration menu
    Copy the full SHA
    d7d81f7 View commit details
    Browse the repository at this point in the history

  2. tsm: cache: add a tests to demonstrate thread safety vulnerabilities 

    There are two tests that show two different one vulnerability.

One test shows that Cache.Deduplicate modifies entries in a snapshot's
store without a lock while cache readers are deduplicating those same
entries while correctly locked.

A second test shows that two threads trying to execute the methods
that Engine.WriteSnapshot calls will cause concurrent, unsynchronized
mutating access to the snapshot's store and entries.

The tests fail at this commit and are fixed by subsequent commits.

Signed-off-by: Jon Seymour <jon@wildducktheories.com>
    @jonseymour
    jonseymour committed Feb 25, 2016
    Configuration menu
    Copy the full SHA
    45d025d View commit details
    Browse the repository at this point in the history

  3. tsm: cache: introduce commit lock to Cache 

    Currently two compactors can execute Engine.WriteSnapshot at once.

This isn't thread safe since both threads want to make modifications to
Cache.snapshot at the same time.

This commit introduces a lock which is acquired during Snapshot() and
released during ClearSnapshot(), ensuring that at most one thread
executes within Engine.WriteSnapshot() at once.

To ensure that we always release this lock, but only release the
snapshot resources on a successful commit, we modify ClearSnapshot() to
accept a boolean which indicates whether the write was successful or not
and guarantee to call this function if Snapshot() has been called.

Signed-off-by: Jon Seymour <jon@wildducktheories.com>
    @jonseymour
    jonseymour committed Feb 25, 2016
    Configuration menu
    Copy the full SHA
    eb7eec0 View commit details
    Browse the repository at this point in the history

  4. tsm: cache: introduce entry locks. 

    Based on @jwilder's alternative to the 'dirty' slice that featured
in previous iterations of this fix.

Suggested-by: Jason Wilder <jason@influxdb.com>
Signed-off-by: Jon Seymour <jon@wildducktheories.com>
    @jwilder @jonseymour
    jwilder authored and jonseymour committed Feb 25, 2016
    Configuration menu
    Copy the full SHA
    452d77c View commit details
    Browse the repository at this point in the history

  5. tsm: cache: remove unnecessary lock escalation. 

    Previously, we needed a write lock on the cache because it was the
only lock we had available to guard updates to entry.values and
entry.needSort.

However, now we have a entry-scoped lock for this purpose, we don't
need the cache write lock for this purpose. Since merged() doesn't
modify the .store or the c.snapshot.sort, there is no need for
a write lock on the cache to protect the cache.

So, we don't need to escalate here - we simply rely on the entry lock
to protect the entries we are iterating over.

Signed-off-by: Jon Seymour <jon@wildducktheories.com>
    @jonseymour
    jonseymour committed Feb 25, 2016
    Configuration menu
    Copy the full SHA
    4d98a1c View commit details
    Browse the repository at this point in the history