On Friday 09 July 2004 11:50 am, Casey Duncan wrote:

On Fri, 9 Jul 2004 16:22:17 +0200 Dieter Maurer <dieter@handshake.de> wrote: [..]

...

The "hasattr" replacement in Python's "__builtin__" could look like:

_marker = [] def hasattr(obj, attr): return getattr(obj, attr, _marker) is not _marker

Hmm. I just heard about this "hasattr geddon". hasattr is *good*. Why are we fixing hasattr and bare excepts when the real problem is ZODB? ZODB should *not* be sensitive to the way the application handles ConflictErrors. When a ConflictError (or ReadConflictError) occurs, the transaction should fall into an "uncommitable" state. From this state, you can only abort the transaction; any attempts to write an object or commit cause another ConflictError. Then, only code that can guarantee that the attempted transaction is complete should actually abort the transaction, and fortunately ZPublisher fits that role. Today, the abort is implicit, and that's the mistake that has caused us to litter the code with knowledge of ConflictErrors. With the "uncommitable" state, it would not matter if the application swallows ConflictErrors. That said, I was surprised to discover that Python 2.3 implements hasattr this way (from bltinmodule.c): v = PyObject_GetAttr(v, name); if (v == NULL) { PyErr_Clear(); Py_INCREF(Py_False); return Py_False; } Py_DECREF(v); Py_INCREF(Py_True); return Py_True; It should not swallow all errors, especially now that descriptors make computed attributes quite common. getattr() only recently started catching only AttributeErrors, but apparently hasattr is lagging behind. I suggest the consistency between getattr and hasattr should be fixed in Python, not Zope. Shane

[Shane Hathaway]

Hmm. I just heard about this "hasattr geddon". hasattr is *good*. Why are we fixing hasattr and bare excepts

Well, bare excepts are generally bad news in any application -- they almost always turn out to catch more than the author intended (including things like SystemExit and KeyboardInterrupt, which are rarely intended to be swallowed).

when the real problem is ZODB? ZODB should *not* be sensitive to the way the application handles ConflictErrors. When a ConflictError (or ReadConflictError) occurs, the transaction should fall into an "uncommitable" state. From this state, you can only abort the transaction; any attempts to write an object or commit cause another ConflictError.

AFAIK, that is the way the current releases of ZODB work. I'm not sure what "write an object" means there, though. I think I know what "commit" means. The logic in Connection.commit() raises ReadConflictError upon an attempt to commit an object that previously suffered a conflict during the transaction. How does that differ from what you want? If you know of a case where this doesn't work, please open a Collector report. I can *believe* there's a problem -- I just don't know of one (intended preconditions and postconditions aren't documented, and the implementation has grown so many clever(?) special cases I'm rarely 100% sure of what it's trying to accomplish).

Then, only code that can guarantee that the attempted transaction is complete should actually abort the transaction, and fortunately ZPublisher fits that role. Today, the abort is implicit, and that's the mistake that has caused us to litter the code with knowledge of ConflictErrors. With the "uncommitable" state, it would not matter if the application swallows ConflictErrors.

That said, I was surprised to discover that Python 2.3 implements hasattr this way (from bltinmodule.c):

v = PyObject_GetAttr(v, name); if (v == NULL) { PyErr_Clear(); Py_INCREF(Py_False); return Py_False; } Py_DECREF(v); Py_INCREF(Py_True); return Py_True;

hasattr has always been like that (that exact code, or a workalike spelling), and that's always how hasattr has been documented to work (the oldest docs I have handy are for Python 1.4).

It should not swallow all errors,

Guido doesn't agree; hoping that Python will change here is probably futile (Guido has said several times that hasattr() should never raise an exception, and that matches how it's always been documented and implemented).

especially now that descriptors make computed attributes quite common. getattr() only recently started catching only AttributeErrors, but apparently hasattr is lagging behind.

The implementation of 2-argument getattr() didn't match the docs until Python 2.2, when someone finally noticed that the implementation didn't match the docs. Then the getattr() implementation was repaired, to match the docs. The docs have never been clear (and still aren't, IMO) about what 3-argument getattr() does with exceptions. I think the current implementation is the most reasonable one, with 3-argument getattr() propagating any exceptions the 2-argument form would have propagated, except for AttributeError -- but the docs don't really say that's what 3-arg getattr() does.

I suggest the consistency between getattr and hasattr should be fixed in Python, not Zope.

They weren't intended to treat exceptions the same way, so an argument based on inconsistency in that area won't make progress. As I said recently in zodb-dev, Zope and ZODB do incredibly complex stuff as side effects in what Guido surely thought of as "tiny hooks". He had in mind that hasattr() might look for a key in a dict or list, and suppress a harmless KeyError or IndexError, not start World War III and then send an endless sequence of Terminators back in time to change the outcome <wink>. BTW, there's a ton of existing code that relies on hasattr() suppressing "things like" KeyError and IndexError, so I doubt Python would change here even if Guido agreed hasattr() was wrong to suppress all exceptions. But that debate belongs on python-dev.

[Shane Hathaway]

Here is what often happens in Zope:

def setFoo(self, value): try: self.foo = value except: LOG("Oops", ERROR, "Some error happened", error=sys.exc_info()) self.change_count += 1 self.get_indexes().update(self)

Some piece of code has a legitimate reason to catch but log all exceptions. Some other piece of code updates indexes. The database has now committed a partial transaction.

I need more words -- I don't see a commit() in that code, and I don't know what "partial transaction" might mean (in ZODB you can commit a transaction, or abort it -- there's no facility I know of for a partial commit (or a partial abort, for that matter)). Is a commit() hiding inside the update() call? If so, and we're assuming the bare "except" swallowed a ReadConflictError, then I have to repeat:

...

The logic in Connection.commit() raises ReadConflictError upon an attempt to commit an object that previously suffered a conflict during the transaction. How does that differ from what you want?

Even worse, this can happen within the indexes, making the indexes inconsistent with themselves.

Sorry, since I didn't understand the first part, I'm way lost here.

Once a conflict error has occurred on any object, the rest of the transaction is on shaky grounds.

Which is why the current ZODB releases intend to prevent committing a transaction if a conflict error occurred during the transaction. It shouldn't matter to this ZODB machinery if the application suppressed the original conflict error(s), ZODB remembers that it raised ReadConflictError regardless (via the Connection._conflicts set-implemented-as-a-dict). Hmm. The lack of design documentation is frustrating. Staring at the code, it could be that ZODB only prevents a transaction commit if the transaction tries to *commit* at least one object that suffered a conflict error, I don't know ... if you have a specific case in mind, please open a Collector report about it.

That's unfortunate. Now I'm inclined to never use hasattr again. Can we petition for "hasattr2" then? :-)

Dieter already wrote it <wink>. Zope/ZODB's use of Python's machinery is sometimes so far from the goals Guido had in mind that Zope is alone in wanting a Python change. For example, I don't know of other apps where calling hasattr() can have disastrous consequences. If so, there's slim constituency for adding another variant of hasattr to the core language. "Does a thing have attribute so-and-so?" is such a mind-numbingly complex question in Zope that I would indeed never use hasattr() in Zope/ZODB code to query general objects. Parts of ZODB 3.3 have been rewritten to use this instead when querying general objects: _marker = object() # The point of this is to avoid hiding exceptions (which the builtin # hasattr() does). def myhasattr(obj, attr): return getattr(obj, attr, _marker) is not _marker That's certainly not needed for "ordinary" uses, like if hasattr(errno, "WSAEWOULDBLOCK"): # Windows Uses of hasattr() on base Python objects and modules is safe (as I believe it is in *almost* all applications outside our part of the Python world).

Tim Peters wrote at 2004-7-10 17:55 -0400:

... Which is why the current ZODB releases intend to prevent committing a transaction if a conflict error occurred during the transaction. It shouldn't matter to this ZODB machinery if the application suppressed the original conflict error(s), ZODB remembers that it raised ReadConflictError regardless (via the Connection._conflicts set-implemented-as-a-dict).

Hmm. The lack of design documentation is frustrating. Staring at the code, it could be that ZODB only prevents a transaction commit if the transaction tries to *commit* at least one object that suffered a conflict error, I don't know ... if you have a specific case in mind, please open a Collector report about it.

This is indeed the case: The code you mentioned prevents the modification (i.e. "commit") of an object that caused a "ReadConflictError" when it has been accessed -- whether or not the exception has been caught or not. Shane seems to want a stricter behaviour: something like: def commit(...): ... if self._conflicts: raise SpecialConflictError(...) i.e. prevent the commit, when the transaction saw any conflict whether or not it tries to modify such an object. However, I think his wish is not necessary. Even when a "ReadConflictError" was swallowed, the object has not been read and its state is still undefined. Whenever the application tries to read an attribute, the object is refetched and the "ReadConflict" reissued. When the conflict successfully trickles down to the "commit", then in fact, the transaction was independent from the state of the respective object... The only thing, the application can do is do write the object. However, "writing" implies fetching the object -- with the consequence of another "ReadConflict" (if I am right, then the code in "commit" is redundant, as the application is unable to write an object that could not be fetched due to a "ReadConflict"). The problem that remains: The application swallowed an exception and, as a consequence, did not recognize that it was unable to *write* an object. It performs its other writes and commits. In this case, the originally written object remains unchanged and inconsistent with respect to other objects. Shanes wish would recognize this situation. Obviously, I have to retract my previous thought :-) -- Dieter