Messages leak at shutdown on windows


#1

On Windows, messages (i.e. MessageManager::MessageBase instances) that get posted to the system queue will leak if JUCE is shut down before they are handled.

The cause is simple: in juce_Win32_messaging.cpp, incReferenceCount() is called manually on a message when it is stuffed into an LPARAM posted to the system queue, and decReferenceCount() is called after it’s handled.If the message window is destroyed before it is handled, Windows discards the pending messages and the reference count is never decreased.

The fix is also simple. Just add a ReferenceCountedArray<MessageManager::MessageBase*, CriticalSection> to this file. Instead of manually managing the reference count, add and remove the message to/from this array. Clear the array at shutdown, just after destroying the message window.

I checked the development branch, it looks like it’s still not fixed there

-Ethan


#2

If the messages had to be added and removed from an array when they are posted/handled, that’s adding a huge amount of overhead for every message. And the heavier the message load, the more the overhead grows, which could bring the system grinding to a halt if the queue gets too busy!

We opted for the pragmatic solution here: add no overhead to the runtime performance, at the expense of a few bytes of memory sometimes being harmlessly leaked on shutdown. If you’re posting huge message objects that would be harmful to leak, you could add your own layer that handles these through the same mechanism you suggest here.


Memory leakage in simple console application
#3

What huge overhead is there exactly in adding/removing from an Array? I was under the impression that your container classes are written to a high standard of performance.

Besides, when a message is posted there is already memory being allocated and freed, and messages being posted to/retrieved form a system queue. Whatever overhead would be added is likely trivial in comparison.

I think we disagree about the meaning of pragmatism. When the situation is:

  1. your library leaks memory
  2. over several years, multiple customers have taken time away from their productive effort to report the leak to you
  3. the fix involves adding two lines of code and changing two lines of code in a single file

I think the pragmatic solution is to stop making excuses and fix it.

-Ethan


#4

if messages are always removed from the top of the queue then you can use an intrusive slist that leads to minimum overhead of one single pointer for the whole queue and one pointer per msg and very small CPU footprint


#5

How dare you accuse me of “making excuses”!!

If the suggestion had actually been sensible, then why would I make an excuse not to do it?? I spent longer answering your post and trying to explain why it’s not practical than it would have taken me to implement it!

It doesn’t matter how efficient the container classes are, this is a big-O-notation question. The current implementation is O(1). Using an array would be O(n), but since n gets higher with load, it could arguably be a higher complexity.

Yes, an intrusive list could be made to work with fairly low overhead, but not zero. It would need to be thread-safe, and messages won’t necessarily be handled in order, which would make it a very tricky custom container to make reliable and fast.

So in my not-so-humble-opinion given decades of experience doing this stuff is that this is a total non-issue that will never cause problems for any user of your software. So it doesn’t justify either a cripplingly-slow solution, or a complex custom data structure that could introduce bugs.


#6

So in my not-so-humble-opinion given decades of experience doing this stuff is that this is a total non-issue that will never cause problems for any user of your software. So it doesn’t justify either a cripplingly-slow solution, or a complex custom data structure that could introduce bugs.

I beg your pardon – I am a user of your software, and this is causing problems for me. That is not up to you to decide, and otherwise I would not be here. Believe me, writing posts here is not my favorite use of time either.

If using an array is not a sensible solution, why do the ios/osx/linux implementations keep messages in an array?


#7

That’s an entirely different situation.

Seriously, it’ll leak something like 8 bytes once per run of your program. In an app it’s impossible for this to ever cause a problem. In a plugin, a user would have to load/unload your plugin millions of times without restarting their DAW before it would even make a visible dent in their memory use.

It seems to me that you must be misunderstanding what’s going on, because you seem to be disproportionately bothered about this!


#8

It seems to me that you must be misunderstanding what’s going on, because you seem to be disproportionately bothered about this!

I agree that this leak will not impact my end users, but again – and I really shouldn’t have to explain this – you do not get to decide what bothers your users, and I am one of your users, and this bothers me.

Specifically it bothers me that every time I’m ending a debugging session I hit an assert, a false positive about some WaitableEvents getting leaked because they are owned by some message that is leaked. And I am treated to a condescending comment about “tut, tut, always use RAII” when I have done nothing wrong and it is in fact JUCE that is not tracking memory correctly and is leaking.

-Ethan


#9

Every one of our thousands of users will be bothered by something different, and we have to decide which ones to deal with.

I’ve only ever had a few people ask about this issue, mostly just in a puzzled way, wondering what’s going on, and once explained, they’ve all generally said “Oh right, I’ll just ignore it”.

But if we added some overhead (which honestly wouldn’t be insignificant) just so that people who are debugging don’t get the spurious warning, then it will genuinely screw up the performance for many other people.

It’s just one of the many similar judgement calls we have to make on a daily basis!


#10

Ok. I do not buy your argument that the overhead would be noticeable to anyone – in practice it would be a small drop in the bucket, and it would already be manifesting on the other platforms – but it’s obvious you have no intention of fixing the leak. Fine, I guess that’s your business.

Given that, I really do want to fix it on my end, and the only way to do that is to modify the events module.

What is the recommended workflow for modifying JUCE modules? I can fix it directly at the installation location, in which case it will get blown away when I update. Or I can tell the projucer I want to import the module into my source tree, but then it seems to blow away my changes when I save the project from the projucer. Am I missing something?

-Ethan


#11

Just use git and merge or rebase your changes.


#12

Of course. I wouldn’t expect anyone to take my opinion seriously, it’s not like I’ve got much experience of how message queues work. It’s not like I’ve spent years sweating over messaging behaviour on every OS that exists.

And sorry, but the team is already so massively busy working on actual useful features that there’s no way I’d ask one of them to spend a few hours implementing and testing something that introduces performance and complexity overhead, and adds zero benefit to anyone’s end-product.

But yes, if it really bothers you, please do hack your own changes to avoid the issue - that’s what git is for!


#13

But yes, if it really bothers you, please do hack your own changes to avoid the issue - that’s what git is for!

So there is no intended workflow here and I just have to make up my own? I can do that, and of course I am using source control anyway. But I’m puzzled by the “copy into source tree” feature. Why is this feature there if the projucer is going to constantly overwrite my changes?

-Ethan


#14

To be honest we should probably remove the old “copy into source tree” option. Its origins date back quite a few years when there were still a significant number of people who were git-deniers and who insisted on getting their juce updates via zip file downloads (!)

Nowadays there’s really no excuse for anyone not to just pull the git repo, and I think we should remove the copying to avoid confusion. (TBH I think the only reason it escaped this long is that we’ve all forgotten it existed…)


#15

So if I’m interpreting you correctly, the intended workflow is that I should just download and update JUCE using git. Ok.

If that’s the case, I’d suggest that when I click on the “Get JUCE” button on the website, I should see something to that effect. Everything about the installation process, and the popups in projucer asking me to update, suggest that JUCE wants me to keep it up to date through other means than git… hence my confusion about how I’m really intended to use the thing.

-Ethan


Timer memory leak?
#16

Fair comment. The website’s due for a big overhaul…


#17

If I recall I think you added the copy function to make each project compiled against Juce “standalone” so that it would work forevermore independently of changes to Juce. This was probably done for people without decent version control and branching practices. It’s the first thing I switched off!