[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: Closing since I believe the bug is fixed and there is an appropriate unit test. -- assignee: -> nascheme resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: Are we missing something here? -- ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: New changeset 392a13bb9346331b087bcd8bb1b37072c126abee by Neil Schemenauer in branch 'master': bpo-38006: Add unit test for weakref clear bug (GH-16788) https://github.com/python/cpython/commit/392a13bb9346331b087bcd8bb1b37072c126abee --

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by STINNER Victor : -- nosy: -vstinner ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.p

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: > While Neil & I haven't thought of ways that can go wrong now > beyond that a "surprise finalizer" may get run any number of > times ... Speaking of which, I no longer believe that's true. Thanks to the usual layers of baffling renaming via macro after macro, I

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: > I'm often amazed it works at all, let alone perfectly. ;-P Indeed! Every time I take a break from gc and come back, I burn another hour wondering why it doesn't recycle _everything_ ;-) > But what happens if the GC doesn't see that WZ is trash? > Then it will

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: > It's fundamentally insane to expect any gc to work perfectly when it may be > blind to what the object graph _is_. I'm often amazed it works at all, let alone perfectly. ;-P This did trigger me to think of another possible problem. I setup my unit test

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by Neil Schemenauer : -- pull_requests: +16348 pull_request: https://github.com/python/cpython/pull/16788 ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: BTW, the phrase "missing tp_traverse" is misleading. If an object with a NULL tp_traverse appears in a gc generation, gc will blow up the next time that generation is collected. That's always been so - gc doesn't check whether tp_traverse is NULL, it just call

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: My understanding is that the CFFI types at issue don't even have Py_TPFLAGS_HAVE_GC. They're completely invisible to gc. As Armin says in the CFFI issue report (linked to earlier), he never got the impression from the docs that he needed to implement anything

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > Or even fatal error out? There's no reason to have Py_TPFLAGS_HAVE_GC but > not implement tp_traverse, AFAIR. Well... It would prefer a smooth transition. Such sanity checks which mostly concerns developers perfectly fit the -X dev mode. I mean, calling

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Antoine Pitrou added the comment: > Would it make any sense to add an opt-in option to emit a warning when a new > type is created with Py_TPFLAGS_HAVE_GC but it doesn't implement tp_traverse? Or even fatal error out? There's no reason to have Py_TPFLAGS_HAVE_GC but not implement tp_travers

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > 1. Docs should be changed to encourage implementing the full gc protocol for > "all" containers. Spell out what can go wrong if they don't. Be upfront > about that history has, at times, proved us too optimistic about that ever > since weakrefs were adde

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Loose ends. Telegraphic because typing is hard. 1. Docs should be changed to encourage implementing the full gc protocol for "all" containers. Spell out what can go wrong if they don't. Be upfront about that history has, at times, proved us too optimistic abo

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Łukasz Langa added the comment: > Łukasz, is there some reason you removed old versions (2.7, 3.6, etc)? The > bug is present on those versions of Python and it should be trivial to > backport the fix. If those branches are maintained, I think we should fix it. Sorry, I did that by mistake.

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: Oh, Neil missed "bpo-38006: " prefix in his commit. Here it is: commit bcda460baf25062ab68622b3f043f52b9db4d21d Author: Neil Schemenauer Date: Mon Sep 30 10:06:45 2019 -0700 Clear weakrefs in garbage found by the GC (#16495) Fix a bug due to t

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: Łukasz, is there some reason you removed old versions (2.7, 3.6, etc)? The bug is present on those versions of Python and it should be trivial to backport the fix. If those branches are maintained, I think we should fix it. Attached is a small test progra

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Neil, about this comment: # - ct is not yet trash (it actually is but the GC doesn't know because of # the missing tp_traverse method). I believe gc should know ct is trash. ct is in the cf list, and the latter does have tp_traverse. What gc won't know i

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Łukasz Langa added the comment: Thanks a million everyone, this is now hopefully solved. I'm leaving it open for Neil to experiment with his unit test (which would be amazing to have!). -- priority: release blocker -> normal versions: -Python 2.7, Python 3.5, Python 3.6, Python 3.7 _

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Łukasz Langa added the comment: New changeset b3612070b746f799901443b65725008bc035872b by Łukasz Langa (Neil Schemenauer) in branch '3.8': Restore tp_clear for function object. (#16502) https://github.com/python/cpython/commit/b3612070b746f799901443b65725008bc035872b --

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: I worked some on trying to create a unit test this evening. Attached is my best result so far (gc_weakref_bug_demo.py). It requires that you enable the 'xx' module so we have a container object without tp_traverse. We should probably add one to _testcap

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by Neil Schemenauer : -- nosy: +benjamin.peterson, larry, ned.deily versions: +Python 2.7, Python 3.5, Python 3.6, Python 3.7 ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: I created GH-16502. I'm not exactly sure of the process of making a revert on a branch like '3.8' so hopefully it is correct. The code change is exactly what has been reverted in 3.8. The net effect will be as if the revert was never done. --

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by Neil Schemenauer : -- pull_requests: +16092 pull_request: https://github.com/python/cpython/pull/16502 ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Łukasz Langa added the comment: If either of you resurrects tp_clear in functions in the next 12 hours or so, I'll merge it. -- ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: > Is introducing tp_clear on functions a thing that has ABI consequences? In > other words, if we take our time on this, would it land in 3.8.1 or 3.9.0? I think it should not have ABI consequences. However, I see the addition of tp_clear as a new feature

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Łukasz, all type objects have tp_clear slots, and always did. The patch in question put something useful in the function object's tp_clear slot instead of leaving it NULL. No interface, as such, changes either way. -- __

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Łukasz Langa added the comment: Now we only need a volunteer to prepare a PR to revert the revert... :> -- ___ Python tracker ___ __

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Yes, it's better to have tp_clear than not for a variety of reasons (including setting examples of best practice). Best I can tell, the patch for BPO-33418 was reverted _only_ to worm around the crash in _this_ report. That's no longer needed. Or, if it is, we

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Łukasz Langa added the comment: Is introducing tp_clear on functions a thing that has ABI consequences? In other words, if we take our time on this, would it land in 3.8.1 or 3.9.0? I'm kind of nervous about the rate of change in the past 48 hours. --

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: > Would [func tp_clear] help with memory usage in functions or was BPO-33418 > addressed in another way since? Having a tp_clear for all container objects that can be involved in reference cycles will help the GC free memory. BPO-33418 may be contrived bu

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: It's unclear to me whether BPO-33418 was a bug or a contrived annoyance :-) If someone believes it was worth addressing, then what it did is the only way to fix it, so should be restored now. -- ___ Python tracker

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Łukasz Langa added the comment: > If that PR is applied, I think we should also restore tp_clear for functions > (revert GH-15826). If that's safe and easy, let's go for it. Would it help with memory usage in functions or was BPO-33418 addressed in another way since? --

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by miss-islington : -- pull_requests: +16088 pull_request: https://github.com/python/cpython/pull/16499 ___ Python tracker ___ __

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: FWIW, I agree with Neil in all respects about the release: his patch is the best approach, plugs segfaulting holes that have been there for many years, and the earlier patches aren't needed anymore. -- ___ Python tra

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Neil, my brief msg 10 minutes before yours suggested the same thing (just clear the weakref), so it must be right ;-) -- ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: Oops, I linked to wrong PR, my proposed fix is GH-16495. It is the same as what Tim suggests in his last comment. -- ___ Python tracker _

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: > Why setting it back to release blocker? As far as I recall, this issue is not > a Python 3.8 regression. The regression which triggered this old and existing > bug has been fixed, see previous comments. I leave it up to our glorious release manager to deci

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Neil, how about this alternative: leave the weakref implementation alone. If we find a trash weakref, simply clear it instead. That would prevent callbacks too, & would also prevent the weakref from being used to retrieve its possibly-trash-too referent. ---

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by Neil Schemenauer : -- pull_requests: +16083 pull_request: https://github.com/python/cpython/pull/16495 ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: > Would the attached rough patch (gc_disable_wr_callback.txt) > be a possible fix? When we find W inside handle_weakrefs(), > we mark it as trash and will not execute the callback. It's semantically correct since we never wanted to execute a callback from a tras

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > 2019-09-30 14:41:36 lukasz.langaset priority: release blocker Why setting it back to release blocker? As far as I recall, this issue is not a Python 3.8 regression. The regression which triggered this old and existing bug has been fixed, see prev

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by Łukasz Langa : -- priority: -> release blocker ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Ah, nevermind my last comment - yes. handle_weakrefs will clear all weakrefs to the objects we know are trash. -- ___ Python tracker ___ ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: > I see that handle_weakrefs() calls _PyWeakref_ClearRef() and that > will clear the weakref even if it doesn't have callback. So, I > think that takes care for the hole I was worried about. I.e. a > __del__ method could have a weakref to an non-valid object. > H

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: > Note that my flags show that W *is* in 'unreachable'. It has > to be otherwise F would not have tp_clear called on it. Right! W holds a strong reference to F, so if W were thought to be reachable, F would be too. But F isn't. > But when delete_garbage() ge

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: > We can have finalizer code running during delete_garbage(). That > code should not have access to non-valid objects. Weakrefs seem be > a way to violate that. handle_weakrefs() take care of some of them > but it seems there are other issues. I see that

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: > Fleshing out something I left implicit: if there's a trash object T > with a finalizer but we don't KNOW it's trash, we won't force-run its > finalizer before delete_garbage starts either. Then, really the same > thing: we may tp_clear some piece of trash

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: Since W is in the unreachable set, we should not be executing its callback. Would the attached rough patch (gc_disable_wr_callback.txt) be a possible fix? When we find W inside handle_weakrefs(), we mark it as trash and will not execute the callback. Th

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: I hacked up my copy of CPython to add flags to PyObject to see the GC state of objects. That makes it easier to know if an object is in the 'unreachable' list or not. > We must know that F is trash, else we never would have called tp_clear(F). Yup. In the

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Fleshing out something I left implicit: if there's a trash object T with a finalizer but we don't KNOW it's trash, we won't force-run its finalizer before delete_garbage starts either. Then, really the same thing: we may tp_clear some piece of trash T's final

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: Sorry, this is very hard for me - broke an arm near the shoulder on Tuesday, and between bouts of pain and lack of good sleep, concentration is nearly impossible. Typing with one hand just makes it worse :-( We must know that F is trash, else we never would hav

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: Victor: > I'm surprised that the function would be seen as "unreachable" if > it's reference counter was equal to 135: If all those references are contained within that garbage set, it is possible. Pablo: > For the weakref to be handled correctly the ctype

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: > call_func and remove are part of a reference cycle. A forced garbage > collection breaks the cycle and removes the two objects, but they are > not removed in the expected order: > > * first: call_func > * then: remove > > The crash requires to destroy the objects

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Tim Peters added the comment: tp_clear implementations are necessary to reclaim trash cycles. They're always best practice for objects that may be in trash cycles. tuples are just "cute rebels" that way ;-) Best guess is that the (some) extension isn't playing by the rules. A weakref ca

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > If I understand Victor's test case correctly, the problem is caused if you > have an extension type that implements tp_traverse but not tp_clear and that > there is also a weakref involved in the trash cycle. I'm not sure that "implements tp_traverse but n

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > Is the behavior of tp_clear the key to this bug? Once func_clear(my_func) is called, calling my_func() will crash: my_func() is unsuable. Because of a complex dance involving borrowed references, the function is called *after* it's cleared. Pablo's PR 15

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Neil Schemenauer added the comment: A few comments from the "peanut gallery". Thanks to Victor and Pablo for doing the hard work of investigating this bug. First, it has been a long time since Tim and I first developed gcmodule.c. So, some of my understanding may be inaccurate due to code

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: I remove the "release blocker" priority since the regression has been fixed in Python 3.8. The main "regression" was the addition of the func_clear() function: I removed it. -- priority: release blocker -> ___ Py

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > I haven't been able to verify that the last patches are sufficient to prevent > Python from segfaulting. We need to wait until rc1 is out. I need a proper > release or RPM build to run the tests on our internal test infrastructure. I pushed two fixes in th

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: There is this PR that avoids a hard crash in the interpreter: https://github.com/python/cpython/pull/15645 -- ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Christian Heimes added the comment: Łukasz, I haven't been able to verify that the last patches are sufficient to prevent Python from segfaulting. We need to wait until rc1 is out. I need a proper release or RPM build to run the tests on our internal test infrastructure. You have to release

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: I reverted the change which added func_clear() to get more time to investigate this bug: New changeset ccaea525885e41c5f1e566bb68698847faaa82ca by T. Wouters (Victor Stinner) in branch '3.8': Revert "bpo-33418: Add tp_clear for function object (GH-8058)" (GH

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: New changeset 23669330b7d0d5ad1a9aac40315ba4c2e765f9dd by Victor Stinner in branch '3.7': bpo-38006: Avoid closure in weakref.WeakValueDictionary (GH-15641) (GH-15789) https://github.com/python/cpython/commit/23669330b7d0d5ad1a9aac40315ba4c2e765f9dd ---

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

miss-islington added the comment: New changeset 78d15faf6c522619098e94be3e7f6d88a9e61123 by Miss Islington (bot) in branch '3.8': bpo-38006: Avoid closure in weakref.WeakValueDictionary (GH-15641) https://github.com/python/cpython/commit/78d15faf6c522619098e94be3e7f6d88a9e61123 -- n

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by STINNER Victor : -- pull_requests: +15442 pull_request: https://github.com/python/cpython/pull/15789 ___ Python tracker ___ __

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by miss-islington : -- pull_requests: +15440 pull_request: https://github.com/python/cpython/pull/15787 ___ Python tracker ___ __

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: New changeset a2af05a0d3f0da06b8d432f52efa3ecf29038532 by Victor Stinner in branch 'master': bpo-38006: Avoid closure in weakref.WeakValueDictionary (GH-15641) https://github.com/python/cpython/commit/a2af05a0d3f0da06b8d432f52efa3ecf29038532 -- ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by Inada Naoki : -- nosy: +inada.naoki ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.py

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: collect() of gcmodule.c: * collect() builds an "unreachable" list which is quite important in this bug * the bug occurs in delete_garbage() which uses the unreachable list * weak references part of unreachable are handled before delete_garbage() in handle_wea

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: gc_crash.py: * PyObject_GC_UnTrack() + PyObject_GC_Track() is used to order objects in the GC collection 0, the objects which are involved in the reference cycle. * BadGC2Type type is implemented in C, uses Py_TPFLAGS_HAVE_GC, implements tp_traverse, but it

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by STINNER Victor : Added file: https://bugs.python.org/file48588/gc_crash.patch ___ Python tracker ___ ___ Python-bugs-list mailing

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: Aha! I reproduced the crash with attached gc_crash.py and gc_crash.patch: # cd /path/to/python/source $ git apply gc_crash.patch $ make $ ./python gc_crash.py Segmentation fault (core dumped) -- Added file: https://bugs.python.org/file48587/gc_crash.p

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: Second step: when func_clear() is called, the closure is cleared. Clearing the closure should call the function which is being cleared (or is already cleared). --- import gc class CallFunc: def __del__(self): func = self.func if func is N

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: I failed to write a reproducer from scratch. So let me share my notes here. The first point is that remove() function of WeakValueDictionary keeps WeakValueDictionary.data alive like that: --- class NoisyDel: def __del__(self): print("dealloc dat

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: Attached crash.tar.gz: minimum reproducer which doesn't depend on FreeIPA anymore. Depends on: * cffi (_cffi_backend dynamic library) * pycparser * Python 3.8 (need "python3.8") To reproduce the crash: $ tar -xf reproducer.tar.gz $ cd reproducer/ $ ./bug.

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Christian Heimes added the comment: Pablo, Victor, could you please assist Armin on the CFFI issue? As usual the situation is more complex than I anticipated. -- ___ Python tracker _

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Christian Heimes added the comment: Thanks Victor! I have opened a bug for CFFI, https://bitbucket.org/cffi/cffi/issues/416/python-38-segfault-cfield_type-does-not -- ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > * A and CB are seen as unreachable by the GC Oh, that's not correct: * Only CB is seen as unreachable by the GC * The GC "clears" CB which makes CB inconsistent (tp_clear) * A is deleted indirectly * Deleting A calls CB through the weak reference to A The

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: This bug is quite complex. Let me try to explain it more simply. * Create an object A * Create a weak reference to A with a callback CB * A and CB are part of a reference cycle * Removing the last references to A and CB are not enough to destroy them * Trigger

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > Another issue is that the GC doesn't prevent the crash. Would it be possible > to prevent the crash without changing the behavior (ex: still call weakref > callbacks)? I discussed with Pablo and it doesn't seem possible to prevent to clear a function if i

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: > Another issue is that the GC doesn't prevent the crash. Would it be possible > to prevent the crash without changing the behavior (ex: still call weakref > callbacks)? Pablo opened bpo-38009 with a PR. I'm not sure if his PR is correct or not yet. ---

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: For the record, here is an annotated traceback of a FreeIPA crash. PyGC_list_contains() is a hack that I wrote to check if an object was in the unreachable argument of delete_garbage(). (gdb) where #0 0x00434b78 in _PyFunction_Vectorcall (func=0x7f

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: I investigated the FreeIPA crash. * Python 3.8 behaves differently because func_clear() has been implemented (bpo-33418, commit 3c452404ae178b742967589a0bb4a5ec768d76e0) * The bug is a crash on a function call (_PyFunction_Vectorcall) because the function h

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Change by Pablo Galindo Salgado : -- Removed message: https://bugs.python.org/msg351022 ___ Python tracker ___ ___ Python-bugs-list

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: Wait, I just checked and the call is done manually: static void ctypedescr_dealloc(CTypeDescrObject *ct) { PyObject_GC_UnTrack(ct); if (ct->ct_weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) ct); I am not sure call to PyObject_

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: For the weakref to be handled correctly the ctypedescr needs to be identified correctly as part of the isolated cycle. If is not in the isolated cycle something may be missing tp_traverse or the GC flags. Can you check if the ctypedescr is part of GC

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Christian Heimes added the comment: The biggest CFFI based dependency of FreeIPA is cryptography, but cryptography does not use weakref in its sources. CFFI on the other hand has a WeakValueDictionary object attached to its internal typecache backend, https://bitbucket.org/cffi/cffi/src/bf80

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: Unless we are missing something I think this may be caused by something in cffi that is not implementing gc-related functions correctly, as PyObject_ClearWeakRefs should not be called from a gc run. Given how complicated this is and even if the possib

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: I think the problem is that whatever is weak-referenced by the weak ref (CField_Type or similar) is not on the gc list. Because is not on the gc list, handle_weakrefs (https://github.com/python/cpython/blob/master/Modules/gcmodule.c#L1090) is not act

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Petr Viktorin added the comment: The traceback does have some ctypedescr_dealloc and cfield_dealloc from _cffi. Could you check what those are? -- ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Christian Heimes added the comment: It's going to be hard to figure that out. FreeIPA uses a ton of C extensions. Some are hand-written, some uses Cython, ctypes, and cffi. -- ___ Python tracker ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: The more I think about this the more I think there is something else at play. If the GC is able to follow the dependency chain, all weakrefs should have been already managed correctly and PyObject_ClearWeakRefs should have never be invoked. I think *

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Petr Viktorin added the comment: > What downsides do we see raising an exception? Yeah, on second thought, that would probably be best. We still want PR #15641 as well, so the exception doesn't actually happen in practice. Note that _PyEval_EvalCodeWithName (the main use of func_globals) alr

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: In https://bugs.python.org/issue33418 I proposed reverting tp_clear on function objects. >What should be done when a function with func_code=NULL is called? We can set the error indicator at least. Although I agree that it seems suboptimal. At least

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: PyFunction_Type.tp_clear changed in bpo-33418 (previously, it was equal to 0: no clear method): commit 3c452404ae178b742967589a0bb4a5ec768d76e0 Author: INADA Naoki Date: Wed Jul 4 11:15:50 2018 +0900 bpo-33418: Add tp_clear for function object (GH-805

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

STINNER Victor added the comment: I'm now able to reproduce the FreeIPA crash. In short: * Get a Fedora Rawhide VM (to get Python 3.8 as "python3", it's more convenient) * Install FreeIPA (dnf install freeipa-server) * Run: ipa-server-install --help * Python does crash at exit > In particula

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Petr Viktorin added the comment: I'm not sure adding a check would solve this. What should be done when a function with func_code=NULL is called? "Silently do nothing" is not really an option; raising an exception wouldn't help much in this case. I wonder if a function's tp_clear we should c

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: See also https://bugs.python.org/issue33418 as the potential source of the problem. -- ___ Python tracker ___ ___

[issue38006] Crash in remove() weak reference callback of weakref.WeakValueDictionary at Python exit

Pablo Galindo Salgado added the comment: In particular, this was not happening before because the function type did not implement tp_clear: https://github.com/python/cpython/blob/3.7/Objects/funcobject.c#L615 The new implementation of tp_clear without checks is allowing this to happen.

  1   2   >