OmniThreadLibrary icon indicating copy to clipboard operation
OmniThreadLibrary copied to clipboard

(Almost) Memory leak when using unobserved nested threads.

Open HHasenack opened this issue 5 years ago • 4 comments

Take a look at my stackoverflow issue where I explain this. Probleem seems to be that "global"omnievent monitor fails to release threads that wrer created by other threads.

https://stackoverflow.com/questions/64459678/

Though I did find a workaround, its a nasty one for your app will keep consuming more and more resources each time threads are started/created. And fastmm wont report a leak because everything is still neatly released when the app closes.

HHasenack avatar Oct 21 '20 11:10 HHasenack

Pretty sure this is a related to #69

HHasenack avatar Oct 27 '20 14:10 HHasenack

I think you are right - there is something fishy with the PostMessage happening in TOmniContainerWindowsMessageObserverImpl.PostWithRetry and the receiving location in TOmniEventMonitor.WndProc.

In your example I even get exceptions as the PostMessage causes GetLastError to give 1400 (handle invalid). In #69 I don't see an error happening from PostMessage but certainly most of those messages go poof somewhere and don't end up in TOmniEventMonitor.WndProc which would call the required Detach(task) which then removes those interface references from the internal dictionary which is responsible for this piling up memory leak.

sglienke avatar Oct 27 '20 17:10 sglienke

The invalid handle (1400) error typically occurs when the master task was completed and its associated Monitor was already destroyed. So - The 'Master' task thread should not die in case there is an "owned" monitor that is monitoring other threads.

So to check this, I changed the timing (using sleep()) to ensure the child task was completed before the master task is completed. The Invalid handle error is gone now, and the COmniTaskMsg_Terminated message gets to be posted successfully. But still the ComniTaskMsg_Terminated from the child task is not processed. (I expected the thread of the MasterTask to handle this.) IMO there are 2 problems:

  1. life time management of the Unobserved monitor
  2. shutdown management of the Unobserved monitor, which should keep the "owning" thread alive and keep processing messages until all monitored threads/tasks are gone.

Also I wonder whether these shutdownmessages hould be handled/processed by the Application main thread (It seems this is the case now) or otherwise through a separate thread that checks all the monitors in GTaskControlEventMonitorPool . AIA, pretty complicated stuff :s...

Giving this some thought, monitors that were created by the application main thread (thus Monitor.ThreadID=MainThreadID) should be handle their messages in the main thread message loop, and all others probably need to be handled by a separate thread... Its just too confusing! I will see if I write a unit test for this, just to demonstrate what I expect to happen.

HHasenack avatar Oct 28 '20 20:10 HHasenack

I have been thinking about the life time end of threads. I came up with the folowing: Wouldnt it be logical that a thread cleans up it’s own task ínterfaces (IOmniTaskCOntrol/IOmniTask) if there are no references to it anymore? This would make threads being “Unobserved” the default. I mean: if I wanted to observe the task, I would have to hold on to the IOmniTaskControl somehow right? Now its the other way around: I have to mark Unobserved, otherwise the IOmniTaskcontrol is destroyed prematurely before the task completes (eg in a pool), and causing an ugly crash. (Unless I am mistaken, which is very well possible.)

HHasenack avatar Jan 04 '21 07:01 HHasenack