[Zope-dev] bare exceptions

[Leonardo Rochael Almeida]

On Fri, 2002-10-04 at 00:13, Casey Duncan wrote:
> As much as I try to avoid them (especially in Zope code), they are sometimes 
> necessary because you simply don't know what exceptions might be raised from 
> inside Python or the standard libs. Besides, even if you stamped it out 
> people will just use:
> 
> try:
>     ...
> except Exception:

in which case we could do as Guido suggested and raise something that
doesn't inherit from Exception :-)

> 
> Besides, sometimes you really do want to trap all exceptions, do something and 
> then re-raise them again. IMO that's not bad form if its not habitual.

Or the ZPublisher case where the error is dealt with by rolling back
transactions, reporting and proceeding as if nothing had happened.
Nobody wants their web app dying just because they didn't think of a
ZeroDivisionError in a PythonScript :-)

But still, those are clear cut cases and the error is not simply ignored
and swept under the carpet.

> Even simple operations in Python itself can raise various exceptions. For 
> instance, a humble int(x) can raise TypeError or ValueError, and Guido knows 
> what else ;^). That leads to me being less confident in my exception handling 
> then I would like to be.

I understand the feeling :-)

> At any rate Chris McDonough and I recently had a conversation about ZPT 
> catching all exceptions and his sneeking suspicion that it was a bad thing 
> with regard to read conflicts, but I think we concluded it wasn't as bad as 
> he thought it might be. Maybe we were wrong.

Well, now we know. It's a bad thing because the requests aren't being
retried at all, and the web app is failing in a very visible way (to the
astonished client) which could be avoided if the exception wasn't
trapped. It's not as bad as it could be, since the code ends up raising
another exception and so the transaction is rolled back and no damage to
the ZODB is done, but compare that with the code in
lib/python/Products/PluginIndexes/common/UnIndex.py:178:

            except:
                LOG(self.__class__.__name__, ERROR,
                    ('unindex_object could not remove '
                     'documentId %s from index %s.  This '
                     'should not happen.'
                     % (str(documentId), str(self.id))), '',
                    sys.exc_info())


This code is trapping read- (and possibly write-) conflict errors,
logging them and proceeding blindly. This could as well be causing the
silent ZCatalog corruptions that were discussed a few days ago! I find
it slightly disturbing that a code that logs 'This should not happen'
just lets the thing that shouldn't be happening go on, and almost
doesn't make any fuss about it! :-)

> Since you have identified these places in the code, I think it would be 
> worthwhile to add the following above the bare excepts in question to see 
> what happens:
> 
> except ReadConflictError:
>     raise
> 
> If nothing else, you could rule this out.

This will certainly solve the problem for the TALES code (with
ConflictError as Chris sugested (or maybe TransactionError or POSError
:-)) but the UnIndex Code looks really b0rked to me. I'm afraid that
leaving a bare exception there (even excluding the ConflictErrors) will
give us trouble in the future (we are very ZCatalog dependant here), but
I don't know enough about the ZCatalog to know what removing it would
entail. I'm also afraid that if I file an issue on it, it will lie there
in the collector forever because no one understands why a bare except
was left there...

Back in the TALES case, I understand the unconditional capturing of
exceptions is done so that another exception can be raised that informs
the user about the line and column (and expression) in ZPT where the
error ocurred, but I don't think that masking the exception with another
exception is the correct way for this. I think the real solution would
be a way to manipulate the traceback so that we can insert meaningful
information about languages we are interpreting without actually losing
the original expression and it's traceback. I picture something along
the lines:

except:
	reraise "TALES error on template %s: line %d column %d" % \
                (expression.template_path, expression.template_line,
		 expression.template_column)

So that this information would be displayed along the right frame of the
traceback instead of replacing it. Maybe it wouldn't even need another
reserved word, a builtin function reraise() or a sys.reraise() could be
enough.

I'm testing a fix for the TALES case along the lines of what Casey
sugested and will report back with results.

Cheers, Leo

-- 
Ideas don't stay in some minds very long because they don't like
solitary confinement.