Skip to content

BUG: Fix leak in the f2py-generated module init and PyMem_Del usage #14217

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

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

superbobry
Copy link
Contributor

The leak is fairly innocent as the created Fortran objects are referenced from the module globals and therefore are only GCed on module reload/interpreter shutdown.

@seberg
Copy link
Member

seberg commented Aug 7, 2019

There are a bunch of similar ones, see gh-12633. Unfortunately, there seems to be some issue/interaction with Python 3.5 (possibly a bug), which means that we could never merge those fixes. It seems that this one strikes the same issue. If you have any idea if/how to circumvent that, that would be awesome.

@superbobry
Copy link
Contributor Author

As an extra twist, the leak is only detected on Python 3.6 (and potentially other 3.X versions) and not on 2.X. We are using an older NumPy version internally but that part is unchanged from the current master.

@seberg
Copy link
Member

seberg commented Aug 7, 2019

Maybe the whole fix has to be applied only to >=3.6 due to the bug or just change in things. That is pretty annoying, but if we can figure out that it is only a few of the places where this is necessary, I suppose we could add some macros/compiler directives to fix it python version dependent...

@seberg
Copy link
Member

seberg commented Aug 7, 2019

Oh, so you found the work around that works on all python versions already? That is awesome, should we continue my old PR? I would could give you access to that branch.

@superbobry
Copy link
Contributor Author

I haven't tested locally on anything other than Python 3.5 and 3.6, but Azure covers 3.7 as well, right?

Feel free to include the PyObject_Free change in gh-12633. Your PR is much more comprehensive than this one.

@seberg
Copy link
Member

seberg commented Aug 8, 2019

I don't care too much either way. I already did that, but my PR is a bit larger (and thus review might be a bit slower). So after some thought I would also be happy to just merge this and rebase mine on top. I assume this is something you actually notice, so fixing it a bit faster may be good.
Can you remove the accidentally added old random files? (Or close if you prefer, if you are happy to review my changes, that would be much appreciated of course. There are a a couple of more I found, although there is also still a reference count leak I have not been able to find yet.)
The test coverage is a bit annoying, since I doubt everything gets covered, but since it is f2py, the coverage tools do not work out of the box...

@charris
Copy link
Member

charris commented Aug 16, 2019

@seberg Ping.

@seberg
Copy link
Member

seberg commented Aug 17, 2019

Ah, I forgot this over my gh-12633, but I can fix this up and merge it tomorrow or so.

@seberg seberg self-requested a review August 17, 2019 02:02
Using `PyMem_Del` is incorrect, it should be `PyObject_Del` here, while
this worked most of the time, it would lead to crashes at least on python
2.5 (after the reference counts were fixed)
@seberg
Copy link
Member

seberg commented Aug 19, 2019

Removed incorrect files, and changed it to tmp to apply my patch easier later. Will merge once tests pass, thanks!

@seberg
Copy link
Member

seberg commented Aug 19, 2019

Had to restart the PyPy test here (hanging process), just noting.

@seberg seberg merged commit 98bdde6 into numpy:master Aug 19, 2019
@seberg seberg changed the title BUG: Fixed a leak in the f2py-generated module initialization code BUG: Fix leak in the f2py-generated module init and PyMem_Del usage Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants