-
Notifications
You must be signed in to change notification settings - Fork 180
Fix: Caching thread safety #752
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
Conversation
mbed/mbed.py
Outdated
try: | ||
if os.path.isfile(lock_file): | ||
with open(lock_file, 'r', 0) as f: | ||
pid = f.read(8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concurrency issue here.
Other proces might have just opened the file, but not yet written to it.
On line 1385 it does:
with open(lock_file, 'wb', 0) as f:
info("Writing cache lock file %s for pid %s" % (lock_file, os.getpid()))
f.write(str(os.getpid()))
f.flush()
os.fsync(f)
So context switch might happen after open()
but just before writing to a file.
This would lead to two processes thinking that they own the lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this would be handled by
if not pid:
if int(os.path.getmtime(lock_file)) + timeout < int(time.time()):
info("Cache lock file exists, but is empty. Cleaning up")
os.remove(lock_file)
os.rmdir(lock_dir)
This means if a lock folder exists and the file has been created and empty (just opened), there will be a 5 minutes time window during which all concurrent processes will be waiting for the file to be written with a valid pid value. This prevents the concurrency issue you described.
Will fix the Py3 issues. Please ignore for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a context manager would improve the pythonishness and reduce the change of future programmer error.
mbed/mbed.py
Outdated
shutil.copytree(cache, path) | ||
self.cache_unlock(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be safer as a context manager? That way we could never forget to unlock the cache.
mbed/mbed.py
Outdated
self.set_cache(url) | ||
self.cache_unlock(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also use the context manager here.
dc2a5ea
to
13ab3f2
Compare
cc: @adamelhakham |
Should help reduce the amount of programmer errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed my own request.
This aims to fix caching thread safety in Mbed CLI. It uses directory creation which is atomic operation on all operating systems. I've tested with the script @adamelhakham wrote. Additionally @adamelhakham tested it in their CI. I'd still advise to try it in staging CI rather than live.
Addresses #660
CC @teetak01 @yogpan01