Function to lock a file using memcache, Version 2

Question

Based on suggested reviews for Function to lock a file using memcache, I have modified the code to

import os
import shutil
import memcache
def addToCart(filename, username):
 ableToLock = memcache.add(filename, username)
 if ableToLock:
 # "ableToLock" can happen if the file is still present
 # or if it was already processed.
 if os.file.ispath(filename):
 # I have a lock and file exists. Think of Cart as a
 # JS Object from where you can pick items to "process".
 return "Added To Cart"
 else:
 # I have a lock but looks like file was processed already.
 # So removing the residual "key" created.
 memcache.delete(filename)
 return "Processed by another user." 
 else:
 # "add" can also fail if remote server is down,
 # but now we are not handling that now. It will
 # block the user's ability to process anything.
 user = memcache.get(filename)
 if user and os.file.ispath(filename):
 # I try my best to show the user processing it.
 print "Being processed by %s" % user
 else:
 # But lost the race to find that.
 print "Processed by another user."
def process(source):
 shutil.move(source, destination)
 filename = os.path.basename(source)
 memcache.delete(filename)

Directly calling process is not considered valid as it can only be done from user's Cart and by that user. If the logic is correct, Cart will only have valid files at any point of time and no two Carts would be having the same file ever. If a request from Cart comes to process a filename and it doesn't exist in the file-system then it is considered flawed.

Are there any race conditions or logical flaws that still exists?

• Sjoerd Job Postmus: To me this code is fairly close to hypothetical code. Maybe if you show a larger example we can give a better review.
• Nishant: I am still on POC of this code. So it is basically the current state of the code. Based on reviews here I would finalize it and then perhaps get reviewed again.
• Nishant: One major flaw I can see if what if memcache itself is down. As their documentation says add can also fail if the remote server is down. So a user can never obtain a lock as opposed, in my case it blocks the productivity at the cost of stopping server errors.

Answer 1

logical flaw

If an exception happens, your key could remain locked.

You use

def addToCart(filename, username):
 ableToLock = memcache.add(filename, username)
 if ableToLock:
 # ableToLock can happen if the file is still present
 # or if it was already processed.
 if os.file.ispath(filename):
 # I have a lock and file exists. Think of Cart as a
 # JS Object from where you can pick items to "process".
 return "Added To Cart"
 else:
 # I have a lock but looks like file was processed already.
 # So removing the residual "key" created.
 memcache.delete(filename)
 return "Processed by another user." 
 else:
 ...
def process(source):
 shutil.move(source, destination)
 # filename => source 
 memcache.delete(filename)

What if shutil.move throws an exception? Or print?

Better would be

if ableToLock:
 try:
 # ableToLock can happen if the file is still present
 # or if it was already processed.
 if os.file.ispath(filename):
 # I have a lock and file exists. Think of Cart as a
 # JS Object from where you can pick items to "process".
 return "Added To Cart"
 else:
 return "Processed by another user." 
 finally:
 memcache.delete(filename)

• Nishant: print is not there, but I got your point. If an exception happens in shutil.move, I am just wondering isn't it okay if the lock is still held on to the file? The user can unlock it at any point. I don't see a flaw in doing that. Ofcourse removing the lock can also be done as an alternative using finally but I guess both are logical options?
• Nishant: I consider it as flawed if a request comes from a user to process one of his locked file and if he ever gets a file not found error.
• Nishant: Do you see any flaws other than unlocking not happening in this scenario? I would prefer to not unlock it in our workflow because it also means I need to remove it from users "Cart" in Javascript. So I am taking one step at a time. Initially just getting a working code with no race conditions.
• Sjoerd Job Postmus: @Nishant: I'm sorry I can't really help you with that. I think there aren't any race conditions. However, detecting race conditions by eye is really difficult, and you should not trust the judgement of a stranger on the internet for this. Also: even if this is free of race conditions, you might inadvertently add another use of locking in a sub-optimal manner and introduce a race condition.
• Nishant: Correct @Sjoerd, thanks for your inputs I will certainly try to simulate this. Your feedbacks are helpful, appreciate that.