5
\$\begingroup\$

I am trying to make file read and update operations thread safe and prevent race conditions in Python. Am I missing out something, or would this work in production?

import os
import time
from contextlib import contextmanager
import errno

current_milli_time = lambda: int(round(time.time() * 1000))
lock_retry_wait = 0.05 # in seconds

@contextmanager
def exclusive_file(filename, mod, lock_wait_timeout=3000):

    lockfile = filename + ".lock"

    wait_start = current_milli_time()

    while True:
        try:
            # acquire lock
            fd = os.open(lockfile, os.O_CREAT|os.O_EXCL)
            break

        except OSError as exception:
            if exception.errno != errno.EEXIST:
                raise

            else:
                t = current_milli_time()
                if (t - wait_start) > lock_wait_timeout:
                    raise

                time.sleep(lock_retry_wait)


    with open(filename, mod) as f:
        yield f

    os.close(fd)
    os.unlink(lockfile)


with exclusive_file("test.txt","a+") as f:
    previous_value = f.read()
    f.truncate()
    f.write(previous_value + str(time.time()))
    time.sleep(20)
\$\endgroup\$
3
\$\begingroup\$

1. Introduction

If you really mean what you say in the post about making access "thread safe", then creating lockfiles is overkill: it would be better to use threading.Lock objects. I am going to assume, in the rest of this review, that you mean "multiprocess safe" rather than "thread safe"

2. Review

  1. There's no documentation. What does exclusive_file do? What guarantees does it provide? What operating systems does it run on?

  2. If code raises an exception while the lock is held, then the lock is not released.

  3. The choice of lock_retry_wait = 0.05 could end up rate-limiting operations involving a file so that they can happen at most 20 times a second. It would be better to make this into a keyword argument to the exclusive_file function, so that a user with different requirements can specify a different value.

  4. It seems perverse that lock_retry_wait is in seconds but lock_wait_timeout is in milliseconds. This seems likely to lead to confusion. It is better for all time values to have the same units.

  5. The current_milli_time function is written with lambda rather than def. There does not seem to be a good reason for this. It's better to use def because you can provide a docstring.

  6. Using Python's built-in datetime.datetime objects, you could avoid the need for the current_milli_time function.

  7. When programming a timeout, it is more efficient to compute a deadline once, in advance, than to repeatedly compute an elapsed time.

  8. When you have a timeout argument to a function, it's often convenient to have a special value (for example None) meaning "keep retrying indefinitely".

  9. Instead of catching all OSError and then re-raising the ones that aren't errno.EEXIST, catch FileExistsError instead.

  10. The built-in function open takes more arguments than mode — there's also buffering, encoding, errors, newline, closefd, opener, and maybe others in forthcoming releases. It would be better for exclusive_file to take *args and **kwargs and forward these to open.

  11. Because exclusive_file is a wrapper around the built-in open, it would be better for it to be named exclusive_open.

  12. The code in the post only works if the process has write access to the directory containing the file. But sometimes you want to exclusively open a file in a directory to which you do not have write access.

3. Revised code

from contextlib import contextmanager
from datetime import datetime, timedelta
import os
from time import sleep

@contextmanager
def exclusive_open(filename, *args, timeout=3, retry_time=0.05, **kwargs):
    """Open a file with exclusive access across multiple processes.
    Requires write access to the directory containing the file.

    Arguments are the same as the built-in open, except for two
    additional keyword arguments:

    timeout -- Seconds to wait before giving up (or None to retry indefinitely).
    retry_time -- Seconds to wait before retrying the lock.

    Returns a context manager that closes the file and releases the lock.

    """
    lockfile = filename + ".lock"
    if timeout is not None:
        deadline = datetime.now() + timedelta(seconds=timeout)
    while True:
        try:
            fd = os.open(lockfile, os.O_CREAT|os.O_EXCL)
            break
        except FileExistsError:
            if timeout is not None and datetime.now() >= deadline:
                raise
            sleep(retry_time)
    try:
        with open(filename, *args, **kwargs) as f:
            yield f
    finally:
        try:
            os.close(fd)
        finally:
            os.unlink(lockfile)

Notes:

  1. I chose to document the issue in 2.12 rather than fixing it.

  2. Possibly the second try: finally: is overkill (close rarely fails), but I think it's wise to make every effort to release a lock. Deadlock is rarely fun to debug.

\$\endgroup\$

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.