Skip to content
This repository was archived by the owner on Feb 4, 2020. It is now read-only.

Cache corruption protection: Atomic replacement of manifests, statistics and object files #286

Merged
frerich merged 7 commits into from
Oct 11, 2017
Merged

Cache corruption protection: Atomic replacement of manifests, statistics and object files #286

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ca1a580
Add atomicRename() and write json files to temporary files before rename
siu Sep 26, 2017
fc01692
Use atomicRename() from copyOrLink
siu Sep 26, 2017
62cbd0f
Read manifests and statistics without locking
siu Sep 26, 2017
cf63867
Re-add touchEntry
siu Sep 26, 2017
41be5a7
Move cache lock inside clearCache, cleanCache and printStatistics
siu Sep 26, 2017
3631edb
Simplify search expression in Manifest.touchEntry(...)
siu Oct 10, 2017
5ca3a14
Reduce duplication with atomicWrite context manager
siu Oct 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 59 additions & 49 deletions clcache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ def normalizeBaseDir(baseDir):
return None


def atomicRename(tempFile, destFile):
os.replace(tempFile, destFile)

@frerich frerich Oct 10, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure this function pulls its own weight; atomicRename is even longer than os.replace and chances are that the latter is already known to readers of the code.

@siu siu Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, replaced by the context manager and os.replace(...)



class IncludeNotFoundException(Exception):
pass

Expand Down Expand Up @@ -157,8 +161,13 @@ def addEntry(self, entry):
"""Adds entry at the top of the entries"""
self._entries.insert(0, entry)

def touchEntry(self, entryIndex):
def touchEntry(self, objectHash):
"""Moves entry in entryIndex position to the top of entries()"""
entryIndex = 0
for i, e in enumerate(self.entries()):
if e.objectHash == objectHash:
entryIndex = i
break

@frerich frerich Oct 10, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think you could shorten this to

entryIndex = next((i, e in enumerate(self.entries()) if e.objectHash == objectHash), 0)

@siu siu Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

self._entries.insert(0, self._entries.pop(entryIndex))


Expand All @@ -177,11 +186,13 @@ def setManifest(self, manifestHash, manifest):
manifestPath = self.manifestPath(manifestHash)
printTraceStatement("Writing manifest with manifestHash = {} to {}".format(manifestHash, manifestPath))
ensureDirectoryExists(self.manifestSectionDir)
with open(manifestPath, 'w') as outFile:
tempManifest = manifestPath + ".new"
with open(tempManifest, 'w') as outFile:
# Converting namedtuple to JSON via OrderedDict preserves key names and keys order
entries = [e._asdict() for e in manifest.entries()]
jsonobject = {'entries': entries}
json.dump(jsonobject, outFile, sort_keys=True, indent=2)
atomicRename(tempManifest, manifestPath)

def getManifest(self, manifestHash):
fileName = self.manifestPath(manifestHash)
Expand Down Expand Up @@ -637,8 +648,11 @@ def __init__(self, fileName):

def save(self):
if self._dirty:
with open(self._fileName, 'w') as f:
tempFileName = self._fileName + ".new"
with open(tempFileName, 'w') as f:
json.dump(self._dict, f, sort_keys=True, indent=4)
atomicRename(tempFileName, self._fileName)

@frerich frerich Oct 10, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Seeing this repetition makes me wonder whether it might be nice to have a little context manager like

@contextlib.contextmanager
def atomicallyWritten(fn):
    tempFileName = fn + ".new"
    with open(tempFileName, 'w') as f:
        yield f
    os.replace(tempFileName, fn)

...so you could just replace open(self._fileName, 'w') with atomicallyWritten(self._fileName). Maybe not worth it, given that it's just two occurrences. I'm mostly thinking out loud.

@siu siu Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It makes sense, let's unify it for the moment, it can be unfolded later on if it is not needed anymore.



def __setitem__(self, key, value):
self._dict[key] = value
Expand Down Expand Up @@ -967,7 +981,7 @@ def copyOrLink(srcFilePath, dstFilePath):
# lower the chances of corrupting it.
tempDst = dstFilePath + '.tmp'
copyfile(srcFilePath, tempDst)
os.rename(tempDst, dstFilePath)
atomicRename(tempDst, dstFilePath)


def myExecutablePath():
Expand Down Expand Up @@ -1392,17 +1406,17 @@ def printStatistics(cache):


def resetStatistics(cache):
with cache.statistics as stats:
with cache.statistics.lock, cache.statistics as stats:

@frerich frerich Oct 10, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If writing the statistics happens atomically now, is it really necessary to lock?

@siu siu Oct 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think so. In general writes need to be protected to avoid other processes updating the information at the same time (based on old information). It is a bit different in this specific part of the code because the resulting statistics is all zeroes but we need to lock anyway because the atomic write always writes first to outputPath + ".new" and we need to prevent two processes doing so at the same time.

We could avoid the lock here by making atomicWrite write to new temporary unique files but I don't think it is worth it at the moment.

@siu siu Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thinking a bit more, even if the temporary file did not always have the same name we would need to lock. We don't want another process to write old information right after we reset the statistics.

@frerich frerich Oct 10, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It's true that the way the temporary file name is currently constructed requires locking. However, I'm not sure I appreciate with your second comment yet. Can you give an example of how two concurrent clcache processes (one of which is merely resetting the statistics) might interact which yields broken statistics in case no locking is performed when resetting the statistics? The only potential issue I could think of was:

  1. clcache instance A reads the statistics in order to bump a counter (e.g. increase the number of cache hits)
  2. clcache instance B resets the statistics
  3. clcache instance A writes out the new statistics, with an increased counter

If this were to happen, the number of cache hits at the end would not be 1 plus whatever the value was before the stats were reset.

However, from looking at the code, all the updates to the cache stats seem to follow the theme

   with cache.statistics.lock, cache.statistics as stats:
            stats.registerCacheHit()

I.e. reading, updating and writing the cache is always an atomic operation.

@siu siu Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Based on your example, instance A is going to register a cache hit and instance B is going to reset the statistics (without the lock):

  1. A: acquires the lock
  2. A: reads current statistics, number of hits = 123
  3. B: resets statistics without acquiring the lock, number of hits on disk = 0
  4. A: writes updated statistics, number of hits on disk = 123 + 1

Resulting in incorrect data. If resetStatistics() acquires the lock this situation is not possible as either A or B would have to wait for the other process to finish the write.

  1. A: acquires the lock
  2. A: reads current statistics, number of hits = 123
  3. B: about to reset statistics, blocks trying to acquire the lock
  4. A: writes updated statistics, number of hits on disk = 123 + 1
  5. A: releases the lock
  6. B: succeeds acquiring the lock, writes all zeros in statistics
  7. B: releases the lock

or

  1. B: acquires the lock
  2. A: about to bump cache hits, blocks trying to acquire the lock
  3. B: writes all zeros in statistics file
  4. B: releases the lock
  5. A: succeeds acquiring the lock
  6. A: reads current statistics, number of hits = 0
  7. A: writes updated number of hits = 0 + 1
  8. A: releases the lock

@frerich frerich Oct 10, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

D'oh, of course you're right. My own example was giving an argument in favor of locking here.

stats.resetCounters()


def cleanCache(cache):
with cache.statistics as stats, cache.configuration as cfg:
with cache.lock, cache.statistics as stats, cache.configuration as cfg:
cache.clean(stats, cfg.maximumCacheSize())


def clearCache(cache):
with cache.statistics as stats:
with cache.lock, cache.statistics as stats:
cache.clean(stats, 0)


Expand Down Expand Up @@ -1508,25 +1522,21 @@ def main():
cache = Cache()

if len(sys.argv) == 2 and sys.argv[1] == "-s":
with cache.lock:
printStatistics(cache)
printStatistics(cache)
return 0

if len(sys.argv) == 2 and sys.argv[1] == "-c":
with cache.lock:
cleanCache(cache)
cleanCache(cache)
print('Cache cleaned')
return 0

if len(sys.argv) == 2 and sys.argv[1] == "-C":
with cache.lock:
clearCache(cache)
clearCache(cache)
print('Cache cleared')
return 0

if len(sys.argv) == 2 and sys.argv[1] == "-z":
with cache.lock:
resetStatistics(cache)
resetStatistics(cache)
print('Statistics reset')
return 0

Expand Down Expand Up @@ -1638,8 +1648,7 @@ def scheduleJobs(cache, compiler, cmdLine, environment, sourceFiles, objectFiles
break

if cleanupRequired:
with cache.lock:
cleanCache(cache)
cleanCache(cache)

return exitCode

Expand All @@ -1661,33 +1670,35 @@ def processSingleSource(compiler, cmdLine, sourceFile, objectFile, environment):
def processDirect(cache, objectFile, compiler, cmdLine, sourceFile):
manifestHash = ManifestRepository.getManifestHash(compiler, cmdLine, sourceFile)
manifestHit = None
with cache.manifestLockFor(manifestHash):
manifest = cache.getManifest(manifestHash)
if manifest:
for entryIndex, entry in enumerate(manifest.entries()):
# NOTE: command line options already included in hash for manifest name
try:
includesContentHash = ManifestRepository.getIncludesContentHashForFiles(
[expandBasedirPlaceholder(path) for path in entry.includeFiles])
manifest = cache.getManifest(manifestHash)
if manifest:
for entryIndex, entry in enumerate(manifest.entries()):
# NOTE: command line options already included in hash for manifest name
try:
includesContentHash = ManifestRepository.getIncludesContentHashForFiles(
[expandBasedirPlaceholder(path) for path in entry.includeFiles])

if entry.includesContentHash == includesContentHash:
cachekey = entry.objectHash
assert cachekey is not None
if entry.includesContentHash == includesContentHash:
cachekey = entry.objectHash
assert cachekey is not None
if entryIndex > 0:
# Move manifest entry to the top of the entries in the manifest
manifest.touchEntry(entryIndex)
cache.setManifest(manifestHash, manifest)
with cache.manifestLockFor(manifestHash):
manifest = cache.getManifest(manifestHash) or Manifest()
manifest.touchEntry(cachekey)
cache.setManifest(manifestHash, manifest)

manifestHit = True
with cache.lockFor(cachekey):
if cache.hasEntry(cachekey):
return processCacheHit(cache, objectFile, cachekey)
manifestHit = True
with cache.lockFor(cachekey):
if cache.hasEntry(cachekey):
return processCacheHit(cache, objectFile, cachekey)

except IncludeNotFoundException:
pass
except IncludeNotFoundException:
pass

unusableManifestMissReason = Statistics.registerHeaderChangedMiss
else:
unusableManifestMissReason = Statistics.registerSourceChangedMiss
unusableManifestMissReason = Statistics.registerHeaderChangedMiss
else:
unusableManifestMissReason = Statistics.registerSourceChangedMiss

if manifestHit is None:
stripIncludes = False
Expand All @@ -1700,22 +1711,21 @@ def processDirect(cache, objectFile, compiler, cmdLine, sourceFile):
includePaths, compilerOutput = parseIncludesSet(compilerResult[1], sourceFile, stripIncludes)
compilerResult = (compilerResult[0], compilerOutput, compilerResult[2])

with cache.manifestLockFor(manifestHash):
if manifestHit is not None:
return ensureArtifactsExist(cache, cachekey, unusableManifestMissReason,
objectFile, compilerResult)

entry = createManifestEntry(manifestHash, includePaths)
cachekey = entry.objectHash
if manifestHit is not None:
return ensureArtifactsExist(cache, cachekey, unusableManifestMissReason,
objectFile, compilerResult)

def addManifest():
entry = createManifestEntry(manifestHash, includePaths)
cachekey = entry.objectHash

def addManifest():
with cache.manifestLockFor(manifestHash):
manifest = cache.getManifest(manifestHash) or Manifest()
manifest.addEntry(entry)
cache.setManifest(manifestHash, manifest)

return ensureArtifactsExist(cache, cachekey, unusableManifestMissReason,
objectFile, compilerResult, addManifest)
return ensureArtifactsExist(cache, cachekey, unusableManifestMissReason,
objectFile, compilerResult, addManifest)


def processNoDirect(cache, objectFile, compiler, cmdLine, environment):
Expand Down
2 changes: 1 addition & 1 deletion unittests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,7 +973,7 @@ def testAddEntry(self):
def testTouchEntry(self):
manifest = Manifest(TestManifest.entries)
self.assertEqual(TestManifest.entry1, manifest.entries()[0])
manifest.touchEntry(1)
manifest.touchEntry("8771d7ebcf6c8bd57a3d6485f63e3a89")
self.assertEqual(TestManifest.entry2, manifest.entries()[0])


Expand Down