On 26/01/2006, at 11:48 PM, Mike Looijmans wrote:
Two comments:
1. (bug): The acquire() call should be *outside* the try ... finally
block. You do not want to release a lock that you did not aquire.
Whoops. Quite agree. One hopes that acquiring a simple mutex lock never
fails though. If it does, you process is probably suffering much more
serious
issues. ;-(
2. (optimization): If you are not planning to change the path, you do
not have to aquire the lock. Aquiring a lock is quite expensive, so
the double-check will have much less impact on performance.
if config.has_key("PythonPath"):
pathstring = config["PythonPath"]
if not _path_cache.has_key(pathstring):
# acquire must be done outside the try block
_path_cache_lock.acquire()
try:
# double-check, because two threads might come
# to the same conclusion up until here.
if not _path_cache.has_key(pathstring):
newpath = eval(pathstring)
_path_cache[pathstring] = None
sys.path[:] = newpath
finally:
_path_cache_lock.release()
You run the very very small risk that two different threads will both
decide
they need to update the sys.path value for the same value of
PythonPath. The
new entries will thus get added twice. This sort of thing was one of the
things wrong with the old code, although now that a map is used to cache
seen values of PythonPath, not so big a problem as simply means you get
redundant entries rather than loss of potential directories from
sys.path.
Number of redundant entries will depend on how many threads hit that
check
at the same time. After the first time it should not happen again.
Thus, trade off of speed versus correctness is probably reasonable as
it will
not cause a failure. In general I tend towards robustness and unexpected
surprises and that is why the code was written as it was. That this
trade
off has been made will need to be clearly documented in code though.
Graham
