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.

• Brian Gesiak: Thanks for the link! So, based on the Python example in the wiki article, in my case the action would be the Receiver, and the manager is the Invoker, and I would make another class with an undo method to act as the Client, correct? I suppose I sort of indadvertedly implemented something like this pattern, as I'm "keep[ing] a stack of the most recently executed commands". How would I better adapt my program to this pattern, and what would be the benefits?
• Winston Ewert: He's already using the command pattern.
• Brady: @modocache, sorry for the delayed response. The answers to your first questions are yes. I didnt see it that clearly when I posted my answer. If you have the opportunity, consider renaming the method/class names to model the design pattern. I think Winston helped answering your last question. If you naturally designed the code to follow the pattern, then that's usually a sign that you are doing things correctly. When I started learning about patterns, there were many that I recognized as having used/implemented without realizing it :)