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

[Bug]: Apparent memory issue? #21788

Open
joelfrederico opened this issue Nov 29, 2021 · 4 comments
Open

[Bug]: Apparent memory issue? #21788

joelfrederico opened this issue Nov 29, 2021 · 4 comments

Comments

@joelfrederico
Copy link
Contributor

Summary

If you insert an exception here:

def __init__(self, canvas, num):
the code has a segfault on quitting. This likely means that there's either a memory leak or double-free or some other memory issue someplace.

Proposed fix

Insert an exception here:

def __init__(self, canvas, num):

Build against a debug version of python and track down the issue using a debugger.

@QuLogic
Copy link
Member

QuLogic commented Dec 18, 2021

The backtrace says it is crashing in [window close] in FigureManager_dealloc. In FigureManager_new, the self->window is allocated, and in FigureManager_init it is initialized using initWithContentRect. If we raise from __init__ immediately, then the super-init is not called and the window is not initialized. Presumably an uninitialized window cannot be closed, but I'm not sure how to check if it has been.

@joelfrederico
Copy link
Contributor Author

joelfrederico commented Dec 18, 2021

It seems like the real issue is a misuse/disuse of tp_alloc, tp_init, tp_finalize, and tp_dealloc.

The window ought to be allocated in a custom tp_alloc. Currently, this is done in tp_init.

The tp_init should call some init method on the window.

The window should be closed in tp_finalize. This should be decoupled from releasing it. I think that can be done by setting isReleasedWhenClosed to false during tp_init.

The window should be released in tp_dealloc.

Is that all correct?

@tacaswell tacaswell added this to the v3.6.0 milestone Dec 18, 2021
@tacaswell
Copy link
Member

@joelfrederico I milestoned this as 3.6 as you did not label this as a regression, but if there is an easy fix to reduce segfaults, we can definitely backport it.

@tacaswell
Copy link
Member

Checking, the state is current the same, however if I am reading this correctly we only have the issue if we raise in the __init__?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants