msg105041 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-05-05 14:30 |
Lines 3884 and 3890 of Objects/typeobject.c (trunk, r80782), which check for a subclass overriding __eq__ (or __cmp__) but not __hash__, call PyErr_WarnPy3k but don't check the return value. [This was reported when compiling Python with clang.] This can lead to an "XXX undetected error", if the warning actually raised an exception:
newton:trunk dickinsm$ ./python.exe -3
Python 2.7b1+ (trunk:80783, May 5 2010, 15:25:42)
[GCC 4.2.1 (Apple Inc. build 5659)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
[35022 refs]
>>> warnings.filterwarnings("error")
[35046 refs]
>>> class A(object):
... def __eq__(self, other):
... return False
...
XXX undetected error
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
[35139 refs]
Nick, I think this came from a checkin of yours: r65642, related to issue 2235. Any suggestions about how to fix it? It's a bit tricky, since the function the check occurs in (inherit_slots) has return type 'void'. Is it okay if I change inherit_slots to return an int here? If so, I'll produce a patch.
Also, can we remove the check related to __hash__ and __cmp__? With __cmp__ gone in 3.1, and 3.0 considered defunct, I don't think it's relevant any more.
|
msg105043 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-05-05 14:49 |
Hmm. Fixing this isn't easy: a simple 'if (inherit_slots(...) < 0) goto error;' produces a huge reference leak in PyType_Ready.
|
msg105071 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2010-05-05 18:52 |
I just came across the warning myself (after ignoring all the PyType_INIT() warnings; what to do about those?) and came to the same conclusion: it's a pain to fix.
One option is to do a PyErr_Occurred() check at inherit_slots() call sites. Would that mitigate the leak? If I remember correctly inherit_slots() is not called constantly (only when a type is being created?) so having the expensive PyErr_Occurred() call should not hurt performance.
|
msg105092 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2010-05-05 22:07 |
I haven't looked at this in a while, but I do remember it was hard to get working at all without a ridiculous number of false alarms - type initialisation isn't the most straightforward thing in the world.
Agreed the warning for __cmp__ should just go away - I think we nuked the actual check in 3.1 when __cmp__ was removed, but must have missed this at the time.
For the refleak, I'm wondering if you may be seeing a problem with statically allocated type objects. type_new does a DECREF on the allocated object if PyType_Ready fails, so everything should be getting cleaned up at that point, but that won't happen for a statically allocated type.
Looking at type_clear, I suggest sticking a "Py_CLEAR(type->tp_mro);" in before the goto error line and see if that eliminates the leak. If that actually works, then I'll have my doubts about the correctness of the other "goto error" lines in PyType_Ready that happen after the call to mro_internal().
|
msg105093 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2010-05-05 22:09 |
(Not sure how relevant my second last paragraph is - I meant to take that out after noticing the MRO details).
|
msg105316 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-05-08 16:50 |
I also have my doubts about the other 'goto error' lines in PyType_Ready, but I haven't figured out how to trigger those gotos in normal code.
Trying to figure out how to clean up properly on error in PyType_Ready is making my head hurt.
As a quick fix, we could simply do a PyErr_Clear() if the PyErr_WarnPy3k returns -1; this clearly isn't right, but at least it gets rid of the "XXX undetected error".
|
msg107133 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-06-05 12:39 |
Removed the __cmp__ warning in r81736, and added a PyErr_Clear() for the __eq__ warning in r81740. I'll leave this open in case anyone wants to figure out how to propagate the warning exception properly.
|
msg107134 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-06-05 12:40 |
Not sure why I added 3.1 and 3.2 to the versions.
|
msg175775 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2012-11-17 17:01 |
A quick search through typeobject.c in 2.7 didn't show any PyErr_WarnPy3k calls that didn't have their return value checked or propagated.
|
msg175787 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-17 17:33 |
There's still a case where the return value isn't properly propagated: it's the one marked by an 'XXX' in the source.
|
msg175789 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2012-11-17 17:37 |
I can't find it, Mark, after searching through every XXX marker in a 2.7 checkout of Objects/typeobject.c .
|
msg175790 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-17 17:39 |
It's line 3912 in the current source. The warning is always cleared, which is the wrong thing to do if warnings should be turned into exceptions.
|
msg175794 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-17 18:07 |
To clarify, here's the bug: the following code should raise an exception, but doesn't:
iwasawa:cpython mdickinson$ ./python.exe -3
Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00)
[GCC 4.2.1 (Apple Inc. build 5664)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings; warnings.filterwarnings("error")
>>> class A(object):
... def __eq__(self, other):
... return False
...
Without "warnings.filterwarnings("error")", the warning gets issued as expected:
iwasawa:cpython mdickinson$ ./python.exe -3
Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00)
[GCC 4.2.1 (Apple Inc. build 5664)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A(object):
... def __eq__(self, object):
... return False
...
__main__:1: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
Brett, is it okay to re-open this? Perhaps a change of title would help? Or I can open a new issue for the remaining problem, if you think that's better.
|
msg175815 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2012-11-17 22:29 |
Go ahead and reopen. I just couldn't find the lines anymore when I closed
it.
On Nov 17, 2012 3:07 PM, "Mark Dickinson" <report@bugs.python.org> wrote:
>
> Mark Dickinson added the comment:
>
> To clarify, here's the bug: the following code should raise an exception,
> but doesn't:
>
> iwasawa:cpython mdickinson$ ./python.exe -3
> Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00)
> [GCC 4.2.1 (Apple Inc. build 5664)] on darwin
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import warnings; warnings.filterwarnings("error")
> >>> class A(object):
> ... def __eq__(self, other):
> ... return False
> ...
>
>
> Without "warnings.filterwarnings("error")", the warning gets issued as
> expected:
>
> iwasawa:cpython mdickinson$ ./python.exe -3
> Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00)
> [GCC 4.2.1 (Apple Inc. build 5664)] on darwin
> Type "help", "copyright", "credits" or "license" for more information.
> >>> class A(object):
> ... def __eq__(self, object):
> ... return False
> ...
> __main__:1: DeprecationWarning: Overriding __eq__ blocks inheritance of
> __hash__ in 3.x
>
> Brett, is it okay to re-open this? Perhaps a change of title would help?
> Or I can open a new issue for the remaining problem, if you think that's
> better.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue8627>
> _______________________________________
>
|
msg228244 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-10-02 17:34 |
May be this patch will fix the issue.
|
msg228327 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2014-10-03 13:12 |
Is there any cleanup to worry about as the comment suggests? If not then the patch LGTM if it does lead to any memory leak.
|
msg228355 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-10-03 17:47 |
How did you detect a leak Mark?
|
msg228358 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2014-10-03 17:54 |
> How did you detect a leak Mark?
Um. It was four years ago. I have no idea. :-)
I suspect I was just looking at the C code and being my usual nasty suspicious self.
|
msg228359 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2014-10-03 17:55 |
Whoops; make that two years ago, not four. The rest stands, though.
|
msg361328 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2020-02-03 21:28 |
Let's close. This was only ever an issue in Python 2.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:00 | admin | set | github: 52873 |
2020-02-03 21:28:50 | mark.dickinson | set | status: open -> closed resolution: out of date messages:
+ msg361328
stage: patch review -> resolved |
2020-01-24 23:27:41 | brett.cannon | set | nosy:
- brett.cannon
|
2017-01-03 06:33:50 | serhiy.storchaka | link | issue29138 superseder |
2014-10-03 17:55:41 | mark.dickinson | set | messages:
+ msg228359 |
2014-10-03 17:54:57 | mark.dickinson | set | messages:
+ msg228358 |
2014-10-03 17:47:05 | serhiy.storchaka | set | messages:
+ msg228355 |
2014-10-03 13:12:49 | brett.cannon | set | messages:
+ msg228327 |
2014-10-02 17:34:57 | serhiy.storchaka | set | files:
+ issue8627.patch
type: behavior components:
+ Interpreter Core versions:
- Python 2.6 keywords:
+ patch nosy:
+ serhiy.storchaka
messages:
+ msg228244 stage: patch review |
2012-11-18 10:34:18 | mark.dickinson | set | status: closed -> open resolution: out of date -> (no value) |
2012-11-17 22:29:48 | brett.cannon | set | messages:
+ msg175815 |
2012-11-17 18:07:01 | mark.dickinson | set | messages:
+ msg175794 |
2012-11-17 17:39:49 | mark.dickinson | set | messages:
+ msg175790 |
2012-11-17 17:37:20 | brett.cannon | set | messages:
+ msg175789 |
2012-11-17 17:33:20 | mark.dickinson | set | messages:
+ msg175787 |
2012-11-17 17:01:46 | brett.cannon | set | status: open -> closed resolution: out of date messages:
+ msg175775
|
2010-06-05 12:40:55 | mark.dickinson | set | messages:
+ msg107134 versions:
- Python 3.1, Python 3.2 |
2010-06-05 12:39:54 | mark.dickinson | set | messages:
+ msg107133 |
2010-05-08 16:50:09 | mark.dickinson | set | messages:
+ msg105316 |
2010-05-05 22:09:51 | ncoghlan | set | messages:
+ msg105093 |
2010-05-05 22:07:38 | ncoghlan | set | messages:
+ msg105092 |
2010-05-05 18:52:53 | brett.cannon | set | nosy:
+ brett.cannon messages:
+ msg105071
|
2010-05-05 14:49:01 | mark.dickinson | set | messages:
+ msg105043 |
2010-05-05 14:30:55 | mark.dickinson | create | |