BUG: Fix leak of void scalar buffer info #12696
Conversation
|
This looks correct to me - Perhaps |
|
Hmmm, that would work too. Can't say I have a preference (on the one hand, the C-level code could be more complex likely. On the other hand, it would have found this issue and probably would just crash hard if there was any reason for not doing it). I somewhat hope this code gets a do-over anyway soon ;). My valgrind run is still going on, but I don't expect anything new from it (although slightly annoying, my cython is again creating leaks although it seems to be 0.29.2, not sure what is going on there). |
|
Sorry, this is still a work in progress for the minute. I just noticed there seems to be one more similar leak, so maybe some other scalar type is missing this after all. |
95bbbd6 to
996d562
Compare
|
Should have looked at the error closer, second one was a bit unrelated after all... |
996d562 to
b49a90c
Compare
The vois scalar was missing the clearing logic for the buffer info cache (or storage). It should maybe be checked whether we can replace all of this logic when not supporting Python 2 anymore.
When the buffer was created and an error was found during the format string construction, the buffer was not freed correctly.
b49a90c to
ae8a979
Compare
|
By the way, I narrowed down the (I think) last definitely lost leak reported by valgrind (except one additional that always shows up, probably due to my python version) to the temporary elide code. I do not think it is a real leak, just if someone ever runs things and wonders what it is: |
|
Mixing up things a bit now, can split up again. Rerunning the test, I had apparently missed two losses earlier. After valgrind, next step might be to try that pytest refcount checker. |
|
Close/reopen. |
|
Btw. after realizing that this manual bisecting was getting seriously annoying. I wrote a hacky pytest plugin: https://github.com/seberg/pytest-valgrind didn't really test it yet, but if anyone ever wants to track down leaks, it should allow to run the full test suit and actually get errors on leaks. It will then tell you which test failed and you can inspect the valgrind log file. In principle we could even expand it a bit and install this on a CI I guess, but valgrind is pretty slow, so I am not sure it should run too often. |
|
Side remarks about that pytest plugin only: Btw. apologies if you someone actually cloned that pytest thing already, yesterday evening, I made some silly changes and then forced pushed :(, will avoid that now. It is a bit annoying, but now you can (and should) pass in a valgrind output log file not just to valgrind but also as an additional pytest argument. In that case it will show the actual valgrind errors in the test results. Plus added markers I am sure all the pytest stuff is a complete hack, but it works and the docs are not super nice (although I could maybe compare with more existing plugins). |
|
Thanks @seberg |
The void scalar was missing the clearing logic for the buffer info
cache (or storage).
It should maybe be checked whether we can replace all of this logic
when not supporting Python 2 anymore.
This is a followup on gh-12624. After this, the test suit should be clean in the sense of valgrind definitely lost leaks (except for the outstanding gh-12650 and more tricky gh-12633 for f2py) [1]. I think if we drop python 2, there may be a reorganizing happening here anyway. Also I am not sure if Matti's changes touch similar things (but doubt it). There is a similar path for
object, but I do not think those really exists?![1] Note that there is still a "leak" (detected, but not true) of one or two buffer info's, because the singletons
np.bool_(True)andnp.bool_(False)get never cleared, and so their cachedbuffer_infoalso does not get cleared.