Thumb maintenance leaks memory [patch attached]

[installgentoo]

Setup (please complete the following information):

• Distribution: gentoo
• Geeqie version [geeqie --version]:Geeqie 2.4 GTK3

Describe the bug
Never frees pixbufs loaded for thumb maintenance -> render thumbs. 16gb OOM at 80k files. Rather serious issue.

Additional context
Patch attached. Editorialise and commit it.

[installgentoo]

memleak.patch

@caclark

[qarkai]

This unrefs fd->thumb_pixbuf which is already happens in file_data_unref() which is called in thumb_loader_free().
Looks like we don't unref FileData somewhere. This patch is only partial solution.
@installgentoo Could you find leaking FileData?

Also, why patch instead of more convenient Pull Request?

[installgentoo]

This unrefs fd->thumb_pixbuf which is already happens in file_data_unref() which is called in thumb_loader_free(). Looks like we don't unref FileData somewhere. This patch is only partial solution. @installgentoo Could you find leaking FileData?

Also, why patch instead of more convenient Pull Request?

No. I fixed what bothered me. Same for pull requests, you guys are maintaining, merge it yourselves.

[installgentoo]

This unrefs fd->thumb_pixbuf which is already happens in file_data_unref() which is called in thumb_loader_free(). Looks like we don't unref FileData somewhere. This patch is only partial solution. @installgentoo Could you find leaking FileData?

Also, why patch instead of more convenient Pull Request?

Also if you are maintaining, you can check my issues, most of them have patches. Open literally for years. Rebase, check, commit.

[qarkai]

I fixed what bothered me.
you can check my issues, most of them have patches. Open literally for years.

Wonder why. Off the top of my head: no one wants to deal with partial fixes.

[installgentoo]

I fixed what bothered me.
you can check my issues, most of them have patches. Open literally for years.

Wonder why. Off the top of my head: no one wants to deal with partial fixes.

That would be a valid complaint if you guys didn't have loads of persistent bugs in your repo. I've found the issue, provided a patch. You can go over the repo with chatgpt codex and ask it to find similar code, then read the code it points you towards. Codex is free. Just don't trust it to write correct code.

Also to your question, i believe geeqie doesn't unref filedata anywhere at all. Filedata is in a pool and gets reused, and that's good actually, you don't want to reload files every time users goes in or out of a directory. I have another open issue with a patch which fixes memory leakage for thumbs, filedata is fine never freeing.

[xsdg]

Howdy. Just to offer some more context, Geeqie does unref FileData, as @qarkai already mentioned, and in the place that he already mentioned. We do generally reload files every time a user leaves and re-enters a directory. Otherwise, we would be holding onto resources that the user is likely to never need again, which is its own form of resource leak.

That said, reaching inside of an fd to unref its private data is 100% not okay. I don't doubt that we have memory leaks (which @qarkai has been doing a great job of finding and fixing). But this is not the solution. This is an easy way to end up with memory corruption or other forms of internal inconsistency in Geeqie's data structures.

[installgentoo]

Howdy. Just to offer some more context, Geeqie does unref FileData, as @qarkai already mentioned, and in the place that he already mentioned. We do generally reload files every time a user leaves and re-enters a directory. Otherwise, we would be holding onto resources that the user is likely to never need again, which is its own form of resource leak.

That said, reaching inside of an fd to unref its private data is 100% not okay. I don't doubt that we have memory leaks (which @qarkai has been doing a great job of finding and fixing). But this is not the solution. This is an easy way to end up with memory corruption or other forms of internal inconsistency in Geeqie's data structures.

I dunno, that actually sounds like you're shittifying geeqie, no wonder it got slower.

On 2.5 fds are in a pool and they aren't reloaded. Reloading fds is idiotic - geeqie lags on opening dirs(because it has to process fds), or if you have a video in the dir, and at least with fds not evicted it lags once. If you eagerly evict fds you're ruining performance.

reaching inside of an fd to unref its private data is 100% not okay

It's 100% okay. fd with nullptr on pixbuff is a valid file with no thumbnail. I understand where you're coming from, but this isn't a java project, and getter/setter things are not "clean code" anyway. Pixbuffs are also refcounted, and things don't "memory leak" from being managed without a setter, they do from being raw fucking pointers. Geeqie have been getting slower as you guys are trying to enforce primitive "clean code" without addressing the shitty asm-style-c architecture.

I may come off as abrasive to you, but you should consider what i've said and re-examine your approach. Empirically, the current attempts to OOP-fy geeqie just made it worse(look at all the "2.7 still slow" issues), without fixing the actual geeqie issues. Because you can't solve architectural problems with getters and setters. Frankly the oopfy trend comes off to me as something from junior engineers.

[installgentoo]

Howdy. Just to offer some more context, Geeqie does unref FileData, as @qarkai already mentioned, and in the place that he already mentioned. We do generally reload files every time a user leaves and re-enters a directory. Otherwise, we would be holding onto resources that the user is likely to never need again, which is its own form of resource leak.

That said, reaching inside of an fd to unref its private data is 100% not okay. I don't doubt that we have memory leaks (which @qarkai has been doing a great job of finding and fixing). But this is not the solution. This is an easy way to end up with memory corruption or other forms of internal inconsistency in Geeqie's data structures.

Also, to prove to you that i'm not only an arrogant asshole, but also know what i'm talking about, look at this issue: #1545

I had the foresight to immediately know that someone will refactor the code and remove fclose(), because it appears like it doesn't do anything besides closing. Colin did not. Might it be possible i am also correct this time?

[xsdg]

At the end of the day, the Geeqie codebase is extremely difficult to maintain because of how code has been added here and there with poor documentation and no real refactors over the decades. The work that @qarkai has been doing is essential to make Geeqie's codebase easier to maintain, which will also make it less likely to trip people up.

I would rather we continue to focus on making Geeqie easier to maintain (even if it gets a bit slower), which will make bigger projects like performance optimizations and the GTK4 transition easier to accomplish with a lower chance of breaking things. If we implement performance optimizations that also introduce complex regressions, that's kind of like one step forward and two steps backward.

[qarkai]

I had the foresight

But chose to not share your knowledge. This sounds like "an arrogant asshole".