Fix crash when restarting OSX single shot timer #9662
Conversation
…b#9655) Previously, if we had a pointer to an old timer when calling start(), we would manually get the context for the old timer and from there get a pointer to the attribute. The problem is that for a single-timer, the timer is automatically invalidated (and context cleared) at the end. This resulted in trying to Py_DECREF(0x0). To fix this, use the built-in support for a callback to run when the context is released. By doing so, we can also clean up a lot of code to release the reference in stop() as well.
Simplifies things just a bit.
Group setting all attributes together. Use NULL (rather than 0) where appropriate for clarity.
|
Anyone wanting to understand some of the changes to the macOS API calls should examine: https://developer.apple.com/documentation/corefoundation/1543570-cfrunlooptimercreate |
|
Here's the script I used for testing: import matplotlib.pyplot as plt
import sys
fig = plt.figure()
timer = fig.canvas.new_timer(interval=100)
timer.single_shot = True
def test():
print(sys.getrefcount(timer._on_timer))
print(sys.getrefcount(timer))
print("TEST")
timer.add_callback(test)
timer.start()
plt.pause(1.0)
timer.start()
plt.pause(1.0)
timer.start()
plt.pause(1.0)
timer.start()
plt.pause(1.0)
timer.stop()Without this PR, the script will crash at the second call to |
|
Hmmm, I get a seg fault and it doesn't appear to hit all the timers? |
|
Works for me - you might need to re-install from source @jklymak to make it work? |
|
Oh I see, a C file changed. Yes this works if I do a |
|
attn @mdehoon ? |
|
Yeah, I'm only barely qualified to tell if this crashes or not - I can't really review it properly. |
|
Merging because it works. If it breaks something else, I suppose the best way to tell is if its in Master... |
|
If something breaks, I promise to pay attention. I'm at least capable of building and running on my osx box. |
|
I use the |
Backport PR #9662 on branch v2.1.x
Fixes #9655.
Previously, if we had a pointer to an old timer when calling start(), we would manually get the context for the old timer and from there get a pointer to the attribute. The problem is that for a single-timer, the timer is automatically invalidated (and context cleared) at the end. This resulted in trying to Py_DECREF(0x0).
To fix this, use the built-in support for a callback to run when the context is released. By doing so, we can also clean up a lot of code to release the reference in stop() as well.
While I was here I also moved a lot of code to use
Py_RETURN_NONE(instead of manually returningPy_None), as well as fixed up some incorrect reference counting.