2
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ To me this code is fairly close to hypothetical code. Maybe if you show a larger example we can give a better review. \$\endgroup\$ – Sjoerd Job Postmus Jun 7 '16 at 6:07
  • \$\begingroup\$ 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. \$\endgroup\$ – Nishant Jun 7 '16 at 8:58
  • \$\begingroup\$ 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. \$\endgroup\$ – Nishant Jun 7 '16 at 9:07
2
\$\begingroup\$

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)
\$\endgroup\$
5
  • \$\begingroup\$ 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? \$\endgroup\$ – Nishant Jun 7 '16 at 9:06
  • \$\begingroup\$ 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. \$\endgroup\$ – Nishant Jun 7 '16 at 9:43
  • \$\begingroup\$ 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. \$\endgroup\$ – Nishant Jun 9 '16 at 5:47
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Sjoerd Job Postmus Jun 9 '16 at 5:53
  • \$\begingroup\$ Correct @Sjoerd, thanks for your inputs I will certainly try to simulate this. Your feedbacks are helpful, appreciate that. \$\endgroup\$ – Nishant Jun 9 '16 at 6:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.