Skip to content
This repository was archived by the owner on Feb 4, 2020. It is now read-only.
Open
Changes from 1 commit
Commits
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
115 changes: 108 additions & 7 deletions clcache.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@
import sys
import multiprocessing
import re
import time
import msvcrt
import random

VERSION = "3.0.3-dev"

Expand Down Expand Up @@ -84,10 +87,9 @@ def __str__(self):
return repr(self.message)


class ObjectCacheLock:
class ObjectCacheLockMutex:
""" Implements a lock for the object cache which
can be used in 'with' statements. """
INFINITE = 0xFFFFFFFF

def __init__(self, mutexName, timeoutMs):
mutexName = 'Local\\' + mutexName
Expand Down Expand Up @@ -126,6 +128,80 @@ def release(self):
self._acquired = False


class ObjectCacheLockFile:
""" Implements a lock file lock for the object cache which
can be used in 'with' statements.
Inspired by
https://github.com/harlowja/fasteners
https://github.com/benediktschmitt/py-filelock"""

def __init__(self, lockfileName, timeoutMs):
self._lockfileName = lockfileName
self._lockfile = None
self._timeoutMs = timeoutMs

def __enter__(self):
self.acquire()

def __exit__(self, typ, value, traceback):
self.release()

def __del__(self):
self.release()

def _acquire(self):
try:
lockfile = open(self._lockfileName, 'a')
except OSError:
lockfile = None
Copy link
Copy Markdown
Owner

@frerich frerich May 23, 2016

Choose a reason for hiding this comment

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

I think this assignment is unneeded: _acquire is only called if self.lockfile == None holds, which is still True in case open raises an exception. I'd say you can just return here. Doing so would permit dropping the explicit else branch and thus lowering the indentation.

else:
try:
msvcrt.locking(lockfile.fileno(), msvcrt.LK_NBLCK, 1)
except (IOError, OSError):
lockfile.close()
else:
self._lockfile = lockfile
return None
Copy link
Copy Markdown
Owner

@frerich frerich May 23, 2016

Choose a reason for hiding this comment

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

For stylistic reason, I think it would be fine to just emit the return statement completely here: the method never returns any value in any of the code paths. I'm not dogmatic about this though.


def _release(self):
if self._lockfile is not None:
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.

This if is both unneded an asymmetric: _release is only called if is_locked() evaluates to True, which is only the case if self._lockfile is not None holds. Furthermore, it's asymmetric, because _acquire doesn't check self._lockfile itself but rather relies on the caller to do so (i.e. _acquire maintains it as a precondition). I'd vote for removing this if to be able to reduce the indentation of the following code block.

lockfile = self._lockfile
self._lockfile = None
msvcrt.locking(lockfile.fileno(), msvcrt.LK_UNLCK, 1)
lockfile.close()

#The following might fail because another instance already has locked the file.
#This is no problem because the existence of the file does not provide the
#locking but win32 api base file locking mechanism.
try:
os.remove(self._lockfileName)
except OSError:
pass
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.

Given that the existence of the file doesn't matter, I guess it could be argued that there's no point in removing the file - how about just dropping this code completely?


return None
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.

Same remarks regarding return None apply here which I mentioned in line 164 above.


def is_locked(self):
return self._lockfile is not None

def acquire(self):
start_time = time.time()
while True:
if not self.is_locked():
self._acquire()

if self.is_locked():
break
elif self._timeoutMs >= 0 and time.time() - start_time > self._timeoutMs/1000:
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 it would be nicer to enforce that self_timeoutMs >= 0 holds right in the constructor such that you don't have to repeatedly check it here.

raise ObjectCacheLockException("Timeout waiting for file lock")
else:
poll_intervall = random.uniform(0.01, 0.1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

camelCase; is this really an interval? I think pollDelay would be a better name indicating that the delay before the next poll – a single float number – is stored.

time.sleep(poll_intervall)

def release(self):
if self.is_locked():
self._release()


class ObjectCache:
def __init__(self):
try:
Expand All @@ -142,7 +218,13 @@ def __init__(self):
os.makedirs(self.objectsDir)
lockName = self.cacheDirectory().replace(':', '-').replace('\\', '-')
timeout_ms = int(os.environ.get('CLCACHE_OBJECT_CACHE_TIMEOUT_MS', 10 * 1000))
self.lock = ObjectCacheLock(lockName, timeout_ms)

cfg = Configuration(self)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is tricky ;) We need to hold a lock before we're allowed to access the configuration. At least this is what the current code suggests:

with cache.lock:

So this might be the point where moving to environment variables makes life easier.

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.

Good catch! There's a hen and egg problem here: the Configuration constructor reads the cache configuration. To do so, you first need to acquire a lock. However, to know what kind of lock to acquire you currently need to look at the cache configuration. Oops.

Quite frankly, given that file-based locking doesn't seem to have any downsides so far (at least not timing-wise), I think we could just as well make it the default for the next version and see how it goes.

if cfg.lockingStrategy() == "File":
lockfileName = os.path.join(self.cacheDirectory(), "cache.lock")
self.lock = ObjectCacheLockFile(lockfileName, timeout_ms)
else:
self.lock = ObjectCacheLockMutex(lockName, timeout_ms)

def cacheDirectory(self):
return self.dir
Expand Down Expand Up @@ -342,7 +424,8 @@ def __contains__(self, key):


class Configuration:
_defaultValues = {"MaximumCacheSize": 1073741824} # 1 GiB
_defaultValues = {"MaximumCacheSize": 1073741824, # 1 GiB
"LockingStrategy": "Mutex"}

def __init__(self, objectCache):
self._objectCache = objectCache
Expand All @@ -358,6 +441,9 @@ def maximumCacheSize(self):
def setMaximumCacheSize(self, size):
self._cfg["MaximumCacheSize"] = size

def lockingStrategy(self):
return self._cfg["LockingStrategy"]

def save(self):
self._cfg.save()

Expand Down Expand Up @@ -888,6 +974,7 @@ def printStatistics(cache):
current cache dir : {}
cache size : {:,} bytes
maximum cache size : {:,} bytes
locking strategy : {}
cache entries : {}
cache hits : {}
cache misses
Expand All @@ -905,6 +992,7 @@ def printStatistics(cache):
cache.cacheDirectory(),
stats.currentCacheSize(),
cfg.maximumCacheSize(),
cfg.lockingStrategy(),
stats.numCacheEntries(),
stats.numCacheHits(),
stats.numCacheMisses(),
Expand Down Expand Up @@ -1165,7 +1253,11 @@ def processCompileRequest(cache, compiler, args):

def processDirect(cache, outputFile, compiler, cmdLine, sourceFile):
manifestHash = ObjectCache.getManifestHash(compiler, cmdLine, sourceFile)
with cache.lock:
postProcessing = None

try:
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.

Not sure I get this change: why couldn't you go with with cache.lock anymore but needed to explicitly acquire/release the lock? Something to do with scoping?

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.

Sorry some temporary change. I revert it. I wanted to try if it is possible to fall back to just invoke the compiler in case we hit a timeout waiting for the lockfile instead of aborting the build.
Before adding the random pollDelay I observed some race conditions, where 12 clcache process were fighting for the lockfile and no one made progress.

cache.lock.acquire()

manifest = cache.getManifest(manifestHash)
baseDir = os.environ.get('CLCACHE_BASEDIR')
if baseDir and not baseDir.endswith(os.path.sep):
Expand Down Expand Up @@ -1196,9 +1288,18 @@ def processDirect(cache, outputFile, compiler, cmdLine, sourceFile):
stripIncludes = True
postProcessing = lambda compilerResult: postprocessNoManifestMiss(cache, outputFile, manifestHash, baseDir, compiler, origCmdLine, sourceFile, compilerResult, stripIncludes)

except ObjectCacheLockException:
printTraceStatement("Timeout waiting for lock")

finally:
cache.lock.release()

compilerResult = invokeRealCompiler(compiler, cmdLine, captureOutput=True)
compilerResult = postProcessing(compilerResult)
printTraceStatement("Finished. Exit code %d" % compilerResult[0])

if postProcessing is not None:
compilerResult = postProcessing(compilerResult)
printTraceStatement("Finished. Exit code %d" % compilerResult[0])

return compilerResult


Expand Down