Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: Fix safeRef regression after removing __cmp__ in #4684 #4896

Merged
merged 1 commit into from
Dec 29, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Dec 28, 2024

After stepping through a lot with PDB, I suggest this to fix the hang described in #4684 (comment) and #4684 (comment)

It isn't a question of missing __le__, __ge__, __ne__ as the replacement of __cmp__ with all rich comparisons. They don't get called. The current (before #4684) __cmp__ method isn't called at all in Python 3 (breakpoint isn't hit, while other do).
In #4684, the __eq__ method gets called by the list.index from the receivers.index(receiver) in here:

def _removeOldBackRefs(senderkey, signal, receiver, receivers):
"""Kill old sendersBack references from receiver
This guards against multiple registration of the same
receiver for a given signal and sender leaking memory
as old back reference records build up.
Also removes old receiver instance from receivers
"""
try:
index = receivers.index(receiver)
# need to scan back references here and remove senderkey
except ValueError:
return False
else:
oldReceiver = receivers[index]
del receivers[index]
found = 0
signals = connections.get(signal)
if signals is not None:
for sig, recs in connections.get(signal, {}).items():
if sig != signal:
for rec in recs:
if rec is oldReceiver:
found = 1
break
if not found:
_killBackref(oldReceiver, senderkey)
return True
return False

Looking at the locals() available in debugging at various stack levels, I see no real difference between having the __eq__ method after #4684, or removing it completly (by memory of what I saw multiple dozens of times per code-change though, so I might be wrong). However, without any extra method, it works, on Linux (ubuntu 22.04 container on WSL) and on windows (editing the saferef.py file in an OSGeo4W grass-dev build 432). I cannot test on macOS, but I suppose it is the same.

This PR effectively has the same behavior as before #4684 for python 3, as that method didn't get called.
From what I searched, it really should have only been called, in python 2, if a list was sorted. I didn't find any other ways the methods in saferef could get triggered.

I consider this change safe.

Additionnaly, in my first attempts to debug this, I also tried raising warnings (similar to DeprecationWarning), with PYTHONWARNINGS=always env var on all methods in saferef, and other methos appeared, but never __cmp__, or the others added in #4684, of course, __eq__ was called, as it was the problematic part

@echoix echoix requested a review from nilason December 28, 2024 18:39
@github-actions github-actions bot added Python Related code is in Python libraries labels Dec 28, 2024
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, thanks!

@nilason nilason added this to the 8.5.0 milestone Dec 29, 2024
@echoix echoix merged commit ce44da6 into OSGeo:main Dec 29, 2024
26 checks passed
@echoix echoix deleted the fix-cmp-saferef-hang branch December 29, 2024 12:59
echoix added a commit to echoix/grass that referenced this pull request Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants