[Patches] [Patch #101277] Catch errors raised by comparisons during dictionary lookup.

Tue, 29 Aug 2000 21:42:52 -0700

Patch #101277 has been updated. 

Project: 
Category: core (C code)
Status: Open
Summary: Catch errors raised by comparisons during dictionary lookup.

Follow-Ups:

Date: 2000-Aug-24 08:44
By: fdrake

Comment:
THIS PATCH IS NOT READY!

This attempts to fix the bug described in SourceForge bug #112558.

In attempting to catch and ignore errors raised by PyObject_Compare(), it checks for those errors, and, if found, clears them and continues to search for another entry.  

Unfortunately, this doesn't quite work.  It fixes the specific case presented in the bug report, but causes other cases to fail.  Running the regression tests for compile, getopt, sre, strftime, and tokenize all fail when PyEval_CallObjectWithKeywords() or eval_code2() detect an error return without an exception being set.

Is should note that errors are detected in these cases when PyObject_Compare() is returning 0; I'm not actually seeing any cases where the comparison is returning non-zero other than the test case in the bug report.

I'd really appreciate some help on this one!
-------------------------------------------------------

Date: 2000-Aug-24 09:00
By: gvanrossum

Comment:
Fine, except that there are two fprintf statements that should be taken out!
After that, check it in and close it, presuming "make test" succeeds.
(I would hope that you have tried this before submitting.)
-------------------------------------------------------

Date: 2000-Aug-24 09:33
By: fdrake

Comment:
(Yes, I knew about the fprintf()s; I figured that's fine as long as the patch was in a testing phase and clearly declared not yet ready.)

Here's a new version of the patch that makes a change to the error detection: it only checks PyErr_Occurred() if PyObject_Compare() returns non-zero.  The regression test passes and the test case in the bug report now does the right thing.

This is based on my vague recollection that the tp_compare function is supposed to return non-zero on errors.  I can't find any documentation about that however.  It would mean that the documentation needs to be updated to state that PyObject_Compare() will return non-zero on errors as well, and code elsewhere that checks for errors after calling PyObject_Compare() may need to be adjusted.

There is a question lurking here:  if an exception is getting set before lookdict() is called or by a comparison returning 0, there's some broken code somewhere else that may need revision.  I'm also led to believe that PyObject_Cmp() may not always report correctly.

So I think this patch is better than no change, but I'm not convinced it's really right.
-------------------------------------------------------

Date: 2000-Aug-24 10:01
By: gvanrossum

Comment:
I don't think this is any better -- it can still call PyErr_Clear when there was a previously existing exception.

Unfortunately you can't use PyObject_Cmp() either, because it does the same thing: just calls PyErr_Occurred().

So the solution is really to use PyErr_Fetch and _Restore...
-------------------------------------------------------

Date: 2000-Aug-24 10:55
By: fdrake

Comment:
Revised to handle Guido's concerns stated in his comments below.  This doesn't avoid the problem of "PyThreadState_Get: no current thread" -- PyErr_Occurred() needs the current thread state to work, and it may not be available.  ;-(
-------------------------------------------------------

Date: 2000-Aug-25 00:54
By: lemburg

Comment:
Fred, have you checked the performance hit this introduces ?

I'm just asking because the code you are touching here is just
about the most often executed in the Python interpreter and I
don't really see the point of adding a performance for situations
which only occur rarely if at all in most programs.

Also, why do you think that masking compare errors during
dict key compare is a good thing ? I agree that the current
situation is bad because it produces dangling execption states,
but simply clearing the exception doesn't help much either --
it only masks other problems. 

The simple and clear solution would be passing the exception
back to the caller instead of clearing it. If you really think that
exceptions during compares should be considered as "doesn't
match", then I'd opt for introducing a whole new PyObject_CompareEx()
API which wraps this idea and takes the appropriate actions.

I think this needs some more discussion on python-dev...

-------------------------------------------------------

Date: 2000-Aug-25 05:49
By: fdrake

Comment:
Performance has *not* been checked -- there's no need to do that until the patch *works*.  I understand the performance issue here; Guido and I have turnned this one around a few times and it's just plain hard to get right.  One thing to recall is that the current implementation is broken as well; try the test case Jeremy submitted when he filed the bug report.

(Keep in mind that I'm calling this patch "NOT READY" still -- I have no intention of checking this in as it stands.)

Clearing exceptions in the comparison is clearly a bad thing, but they're getting raised in a chunk of code that simply doesn't allow raising exceptions.  This is not well explained in the comments; I'll try to improve them.  One issue with the problem is that and exception in the comparison can cause us not to find the entry we're looking for; during C-level exception handling, we're still trying to propogate another exception as well.

Even if the general solution proves too slow for use with the module dictionaries (which will require testing to determine), I have some ideas about what it would take to make those fast anyway.  I'll describe those later on python-dev.
-------------------------------------------------------

Date: 2000-Aug-25 07:12
By: lemburg

Comment:
Ok. 

Sorry if that last message sounded a bit like FUD. 

This needs to be fixed, but I do think that we should get some more feedback
and ideas into this... so let's head for python-dev :-)

-------------------------------------------------------

Date: 2000-Aug-29 20:49
By: fdrake

Comment:
Please review and advise regarding the importance of this bug.

Not that this patch may have some problems; appearantly Guido was able to get it to break (missing tstate needed by PyErr_*() usage introduced here), but I've not received information needed to duplicate the breakage.
-------------------------------------------------------

Date: 2000-Aug-29 21:21
By: tim_one

Comment:
FYI, Guido told me the patch (or a close relative) made it thru the std test suite, but died running pystone(!).
-------------------------------------------------------

Date: 2000-Aug-29 21:42
By: fdrake

Comment:
Guido told me that as well, but I was not able to reproduce it.  He has not provided instructions to duplicate the failure; I just ran pystones.py as a script.
-------------------------------------------------------

-------------------------------------------------------
For more info, visit:

http://sourceforge.net/patch/?func=detailpatch&patch_id=101277&group_id=5470