Performing a chain of mutually dependent actions, and reversing in case of failure

Question

Description

I'm writing a program that makes changes to a few Python source files--creating some, and appending content to others. Since some changes depend on others, I check frequently to make sure everything is going smoothly (try importing the modified file, etc). I would like to revert any and all changes my program has made if it encounters a problem.

I decided the best way of going about doing this would be to use an Action class, with methods to perform and reverse the changes (like migrations in Django's South, or Rails migrations). An ActionManager class performs each action in turn, and if any fail, reverts them in the reverse order.

Dilemma

The code I wrote appears to be working fine. However, I have three problems with it:

1. The section marked ??? in the code below seems like it is faulty. Are there any conditions where this would break? If so, is there a better way?

2. I am not 100% sure all changes will be reversed if, say, an exception occurs. If possible, I want to make sure all changes are reverted even if the program is interrupted midway. I believe my exception handling is flimsy at best, faulty at worst. Any advice appreciated.

3. The API is atrocious. Specifying a manager every time an action is initialized seems tedious, and so does calling super in every action method. Is there a more elegant solution?

Any and all advice on the above or anything else in the source below is appreciated.

Code

Base Classes

#-*- coding: utf-8 -*-

import errno
import os
import shutil
import tempfile


class FileAction(object):
    """A reversible action performed on the filesystem."""

    def __init__(self, mgr, origfile):
        self.manager = mgr
        self.manager.pending_actions.append(self)
        self.origfile = origfile

    def execute(self):
        """Perform action.

        Return a Boolean indicating whether action
        was successful or not.
        """
        filename = os.path.basename(self.origfile)
        # ??? - Append object id to filename in order to make sure
        # two actions can modify the same file without colliding.
        action_id = str(id(self))
        dst = os.path.join(self.manager.tmpdir, filename + action_id)

        if os.path.isfile(dst):
            raise OSError(
                errno.EPERM, 'File already exists at {0}.'.format(dst))
            return False
        else:
            shutil.copy2(self.origfile, dst)
            self.tmpfile = dst

        return True

    def revert(self):
        """Undo action."""
        shutil.copy2(self.tmpfile, self.origfile)

    def cleanup(self):
        """Remove any temporary files."""
        try:
            os.remove(self.tmpfile)
        except:
            raise
            return False
        else:
            return True


class FileActionManager(object):
    def __init__(self):
        """Create temporary directory used by actions."""
        self.executed_actions = []
        self.pending_actions = []

    def execute(self):
        """Perform all actions, return Boolean indicating their success."""
        self.tmpdir = tempfile.mkdtemp()

        for action in self.pending_actions:
            is_success = action.execute()
            self.pending_actions.remove(action)
            self.executed_actions.append(action)

            if not is_success:
                self.revert()
                return False

        return self.cleanup()

    def revert(self):
        return all([a.revert() for a in reversed(self.executed_actions)])

    def cleanup(self):
        """Remove all temporary files."""
        is_success = all([a.cleanup() for a in self.executed_actions])

        try:
            os.rmdir(self.tmpdir)
        except:
            raise
            return False
        else:
            if is_success:
                self.executed_actions = []
                self.pending_actions = []
            return is_success

Sample Usage

#!/usr/bin/env python
#-*- coding: utf-8 -*-

import time

from actionmanager import FileAction, FileActionManager


class AppendTimestampAction(FileAction):
    def execute(self):
        if super(AppendTimestampAction, self).execute():
            with open(self.origfile, 'a') as f:
                f.write(str(time.time()) + '\n')
            return True
        return False


class FailingAction(FileAction):
    def execute(self):
        super(FailingAction, self).execute()
        return False


def main():
    target_file = 'target.txt'
    mgr = FileActionManager()

    AppendTimestampAction(mgr, target_file)
    AppendTimestampAction(mgr, target_file)
    mgr.execute() # => file includes two timestamps

    AppendTimestampAction(mgr, target_file)
    FailingAction(mgr, target_file)
    mgr.execute() # => file has timestamp appended, but is then reverted due to failure


if __name__ == '__main__':
    main()

Answer 1

#-*- coding: utf-8 -*-

import errno
import os
import shutil
import tempfile


class FileAction(object):
    """A reversible action performed on the filesystem."""

    def __init__(self, mgr, origfile):

I'd avoid abbrevations like mgr and origfile just use the full names

        self.manager = mgr
        self.manager.pending_actions.append(self)
        self.origfile = origfile

    def execute(self):
        """Perform action.

        Return a Boolean indicating whether action
        was successful or not.
        """
        filename = os.path.basename(self.origfile)
        # ??? - Append object id to filename in order to make sure
        # two actions can modify the same file without colliding.

id is guaranteed to be unique for living objects. The only way you get could a collision is if an object was destroyed. But that won't happen in your case here. However, id is a performance problem on some alternative versions of python, such as PyPy. I'd keep a count on your manager object, and use that to assign numbers.

        action_id = str(id(self))
        dst = os.path.join(self.manager.tmpdir, filename + action_id)

        if os.path.isfile(dst):
            raise OSError(
                errno.EPERM, 'File already exists at {0}.'.format(dst))
            return False

You seem confused about how exceptions work. You can either have an exception or a return value, you can't have both. The exception skips the rest of the function, the return will never be executed

        else:
            shutil.copy2(self.origfile, dst)
            self.tmpfile = dst

        return True

    def revert(self):
        """Undo action."""
        shutil.copy2(self.tmpfile, self.origfile)

    def cleanup(self):
        """Remove any temporary files."""
        try:
            os.remove(self.tmpfile)
        except:
            raise

Don't catch exceptions just to reraise it.

            return False
        else:
            return True

This code is equivalant to

os.remove(self.tmpfile)
return True    

However, you probably don't actually need to return anything. Just use exceptions to indicate errors.

class FileActionManager(object):
    def __init__(self):
        """Create temporary directory used by actions."""
        self.executed_actions = []
        self.pending_actions = []

    def execute(self):
        """Perform all actions, return Boolean indicating their success."""
        self.tmpdir = tempfile.mkdtemp()

        for action in self.pending_actions:
            is_success = action.execute()

You don't handle the exceptions this might throw. In fact, is_success will never actually be False since in those cases exceptions are thrown.

            self.pending_actions.remove(action)

You shouldn't modify a list while you are executing over it. You might want to do something like:

while self.pending_actions:
    action = self.pending_actions.pop(0)
    ...

That way iteration is safe, and the objects are removed from the list naturally

            self.executed_actions.append(action)

If the action failed, should you record it as executed and then try to revert it?

            if not is_success:
                self.revert()
                return False

        return self.cleanup()

    def revert(self):
        return all([a.revert() for a in reversed(self.executed_actions)])

Using an expression like this for side-effects is confusing. List comprehensions are usually not used for performing actions.

    def cleanup(self):
        """Remove all temporary files."""
        is_success = all([a.cleanup() for a in self.executed_actions])

        try:
            os.rmdir(self.tmpdir)
        except:
            raise
            return False

Again, this doesn't work.

        else:
            if is_success:
                self.executed_actions = []
                self.pending_actions = []
            return is_success

The biggest point is that you clearly don't understand how exceptions operate. You'll want to review that.

While the command pattern is a useful implementation technique, its an annoying api. A better api would look something like

with FileTransaction() as transaction:
    with transaction.open("foo.txt","a") as foo:
         foo.write("Something to append")
    with transaction.open("bar.txt") as bar:
         bar.write("new content")

    raise Exception("Oops!")

Where the open method looks something like

def open(filename, mode):
    action = FileAction(filename, mode)
    action.pre_execute() # makes the temporary copy of the file
    self.actions.append(action)
    return open(filename, mode)

By making your FileTransaction() object a context manager, you can detect when you leave the if block in an exception. In that case, go through self.actions and revert all the changes. If you leave without an exception, commit all the changes.

Don't use return True or return False to indicate errors. Only use exceptions to indicate failures.

I decided to take a stab at implementing my approach. Here is what results:

import tempfile
import os.path
import shutil

class FileModification(object):
    def __init__(self, transaction, filename):
        self.transaction = transaction
        self.filename = filename
        self.backup_path = None

    def execute(self):
        self.backup_path = self.transaction.generate_path()
        shutil.copy2(self.filename, self.backup_path)

    def rollback(self):
        shutil.copy2(self.backup_path, self.filename)

    def commit(self):
        os.remove(self.backup_path)

class FileCreation(object):
    def __init__(self, transaction, filename):
        self.transaction = transaction
        self.filename = filename

    def execute(self):
        pass

    def commit(self):
        pass

    def rollback(self):
        os.remove(self.filename)

class FileTransaction(object):

    def __init__(self):
        self.log = []
        self.counter = 0
        self.temp_directory = tempfile.mkdtemp()

    def generate_path(self):
        self.counter += 1
        return os.path.join(self.temp_directory, str(self.counter))

    def rollback(self):
        for entry in self.log:
            entry.rollback()

    def commit(self):
        for entry in self.log:
            entry.commit()

    def __enter__(self):
        return self

    def __exit__(self, error_type, error_value, error_traceback):
        if error_type is None:
            self.commit()
        else:
            self.rollback()

    def open(self, filename, mode):
        if os.path.exists(filename):
            modification = FileModification(self, filename)
        else:
            modification = FileCreation(self, filename)
        modification.execute()
        self.log.append(modification)
        return open(filename, mode)

A few things could still use some help here:

1. What if an exception is raised by a rollback or commit method?
2. What if the user calls os.chdir()?
3. What if the user neglects to close a file until have the transaction has been rolled back or comitted?

Answer 2

A common way to do this sort of thing is with the Command pattern, implementing Undo.