zopefoundation / zope.interface

Interfaces for Python
http://zopeinterface.readthedocs.io/
Other
330 stars 71 forks source link

fix: call 'PyObject_ClearWeakRefs' unconditionally #324

Closed tseaver closed 1 week ago

tseaver commented 1 month ago

Toward #323.

@stollero Could you test with this patch to see if it fixes #323?

tseaver commented 1 month ago

@davisagli

Do we also need to call PyObject_ClearWeakRefs in the tp_dealloc functions for the other types that use BASETYPE_FLAGS?

7ca6275 tries to comply across the board. PTAL.

davisagli commented 1 month ago

@tseaver The tests pass, but some errors are logged when I run tox -e py311:

Exception ignored in tp_clear of: <class 'dict'>
Traceback (most recent call last):
  File "/Users/davisagli/Plone/zope.interface/src/zope/interface/ro.py", line 289, in __init__
    def __init__(self, C, memo):

SystemError: Objects/weakrefobject.c:951: bad argument to internal function
Exception ignored in tp_clear of: <class 'dict'>
Traceback (most recent call last):
  File "/Users/davisagli/Plone/zope.interface/src/zope/interface/ro.py", line 289, in __init__
    def __init__(self, C, memo):

SystemError: Objects/weakrefobject.c:951: bad argument to internal function
...................................................Exception ignored in tp_clear of: <class 'dict'>
Traceback (most recent call last):
  File "/Users/davisagli/Plone/zope.interface/src/zope/interface/ro.py", line 247, in __init__
    def __init__(self, C, mro):

SystemError: Objects/weakrefobject.c:951: bad argument to internal function

That comes from here: https://github.com/python/cpython/blob/3.11/Objects/weakrefobject.c#L951

Apart from SB, the other types only have Py_TPFLAGS_MANAGED_WEAKREF (via BASETYPE_FLAGS) when USE_EXPLICIT_WEAKREFLIST is 0 (in Python 3.12+) so we should probably also use that condition for calling PyObject_ClearWeakRefs here.

Or else take Py_TPFLAGS_MANAGED_WEAKREF back out of BASETYPE_FLAGS and only add it for SpecificationBase

dataflake commented 4 weeks ago

@tseaver knock knock - do you have a little time to finish this?

davisagli commented 2 weeks ago

@tseaver @dataflake I updated this branch with my second suggestion from above, which resolves the errors I mentioned.

dataflake commented 1 week ago

I can't say much about the C code changes because that's not my expertise, but if it fixes the issues it's OK for merging.

tseaver commented 1 week ago

@davisagli 2524dc6fdbc034db1908c94c53fa8497a5d3b48e LGTM -- thanks for taking up my slack!

mauritsvanrees commented 1 week ago

Plone gives a segmentation fault when used with Zope 5.11. This PR fixes it. Can we have a release please?

I might be able to make a release myself, at least I should have PyPI rights. But I am not sure how that goes with the C code, so I might unknowingly break something.

BTW, after Zope 5.11 was released I reported that the Plone tests passed, but apparently this segfault managed to hide itself...

dataflake commented 1 week ago

I cannot do a release, I am traveling without access to a computer.

mauritsvanrees commented 1 week ago

I wanted to try, but apparently I don't have rights to upload to PyPI anyway, which surprises me as I am part of the zope pypi organisation. Oh well, I can wait.

davisagli commented 1 week ago

@mauritsvanrees @dataflake I released zope.interface 7.1.1