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.