Posts Tagged ‘exceptions’
A little while ago I was asked what my biggest gripe with Django was. At the time I couldn’t think of a good answer because since I started using Django in the pre-1.0 days most of the rough edges have been smoothed. Yesterday though, I encountered an error that made me wish I thought of it at the time.
The code that produced the error looked like this:
from django.db import models class MyModel(model.Model): ... def save(self): models.Model.save(self) ... ...
The error that was raised was AttributeError: 'NoneType' object has no attribute 'Model'. This means that rather than containing a module object, models was None. Clearly this is impossible as the class could not have been created if that was the case. Impossible or not, it was clearly happening.
Adding a print statement to the module showed that when it was imported the models variable did contain the expected module object. What that also showed was that module was being imported more than once, something that should also be impossible.
After a wild goose chase investigating reasons why the module might be imported twice I tracked it down to the load_app method in django/db/models/loading.py. The code there looks something like this:
def load_app(self, app_name, can_postpone=False): try: models = import_module('.models', app_name) except ImportError: # Ignore exception
Now I’m being a harsh here, and the exception handler does contain a comment about working out if it should reraise the exception. The issue here is that it wasn’t raising the exception, and it’s really not clear why. It turns out that I had a misspelt module name in an import statement in a different module. This raised an ImportError which was caught, hidden and then Django repeatedly attempted to import the models as they were referenced in the models of other apps. The strange exception that was originally encountered is probably an artefact of Python’s garbage collection, although how exactly it occurred is still not clear to me.
There are a number of tickets (#6379, #14130 and probably others) on this topic. A common refrain in Python is that it’s easier to ask for forgiveness than to ask for permission, and I certainly agree with Django and follow that most of the time.
I always follow the rule that try/except clauses should cover as little code as possible. Consider the following piece of code.
try: var.method1() var.member.method2() except AttributeError: # handle error
Which of the three attribute accesses are we actually trying to catch here? Handling exceptions like this are a useful way of implementing Duck Typing while following the easier to ask forgiveness principle. What this code doesn’t make clear is which member or method is actually optional. A better way to write this would be:
var.method1() try: member = var.member except AttributeError: # handle error else: member.method2()
Now the code is very clear that the var variable may or may not have a member member variable. If method1 or method2 do not exist then the exception is not masked and is passed on. Now lets consider that we want to allow the method1 attribute to be optional.
try: var.method1() except AttributeError: # handle error
At first glance it’s obvious that method1 is optional, but actually we’re catching too much here. If there is a bug in method1 that causes an AttributeError to raised then this will be masked and the code will treat it as if method1 didn’t exist. A better piece of code would be:
try: method = var.method1 except AttributeError: # handle error else: method()
ImportErrors are similar because code can be executed, but then when an error occurs you can’t tell whether the original import failed or whether an import inside that failed. Unlike with an AttributeError there is a no easy way to rewrite the code to only catch the error you’re interested in. Python does provide some tools to divide the import process into steps, so you can tell whether the module exists before attempting to import it. In particular the imp.find_module function would be useful.
Changing Django to avoid catching the wrong ImportErrors will greatly complicate the code. It would also introduce the danger that the algorithm used would not match the one used by Python. So, what’s the moral of this story? Never catch more exceptions than you intended to, and if you get some really odd errors in your Django site watch out for ImportErrors.
Recently I was taking part in a review of some Python code. One aspect of the code really stuck out to me. It’s not a structural issue, but a minor change in programming style that can greatly improve the maintainability of the code.
The code in general was quite good, but a code snippet similar to that given below jumped right to the top of my list of things to be fixed. Why is this so bad? Let us first consider what exceptions are and why you might use them in Python.
try: // code except Exception, e: // error handling code
Exceptions are a way of breaking out the normal program flow when an ‘exceptional’ condition arises. Typically this is used when errors occur, but exceptions can also be used as an easy way to break out of normal flow during normal but unusual conditions. In a limited set of situations it can make program flow clearer.
What does this code do though? It catches all exceptions, runs the error handling code and continues like nothing has happened. In all probability it’s only one or two errors that are expected and should be handled. Any other errors should be passed on a cause the program to actually crash so it can be debugged properly.
Let’s consider the following code:
analysis_type = 1 try: do_analysis(analysis_typ) except Exception, e: cleanup()
This code has a bug, the missing e in the do_analysis call. This will raise a NameError that will be immediately captured and hidden. Other, more complicated errors could also occur and be hidden in the same way. This sort of masking will make tracking down problems like this very difficult.
To improve this code we need to consider what errors we expect the do_analysis function to raise and what we want to handle. In the ideal case it would raise an AnalysisError and then we would catch that.
analysis_type = 1 try: do_analysis(analysis_typ) except AnalysisError, e: cleanup()
In the improved code the NameError will pass through and be picked up immediately. It is likely that the cleanup function needs to be run whether or not an error has occurred. To do that we can move the call into a finally block.
analysis_type = 1 try: do_analysis(analysis_typ) except AnalysisError, e: // display error message finally: cleanup()
This allows us to handle a very specific error and ensure that we clean up whatever error happens. Sometimes cleaning up whatever the exception (or in the event of no exception) is required, and in this case the finally block, which is always run, is the right place for this code.
Let’s now consider a different piece of code.
try: do_analysis(analysis_types[index]) except KeyError: // display error message
We’re looking up the parameter to do_analysis in a dictionary and catching the case where index doesn’t exist. This code is also capturing too much. Not because the exception is too general, but because there is too much code in the try block.
The issue with this code is what happens if do_analysis raises a KeyError? To capture the exceptions that we’re expecting we need to only wrap the dictionary lookup in and not catch anything from the analysis call.
try: analysis_type = analysis_types[index] except KeyError: // display error message finally: do_analysis(analysis_type)
So, if I’m reviewing your code don’t be afraid to write a few extra lines in order to catch the smallest, but correct, set of exceptions.