Delayed Action, revisited
A few days ago, I posted a simple “delayed action” unit that allows you to post a simple action to the Windows Message Queue, to be executed on the main thread after all currently pending messages. It worked by storing your proc in its own threadsafe queue, then posting a message which, when read, caused the proc to be popped back off the queue and executed.
Turns out there are a couple problems with that. First, the message is already going through the Windows Message Queue. As David Heffernan pointed out in the comments, “One queue is enough. Two is too many.” I did it that way to have something to hold a reference to the anonymous method. But it turns out that that’s not necessary. You can take a pointer to the anonymous method, and the Delphi compiler will automatically set up the proper reference counting for you.
Second, not only is using an internal queue aesthetically displeasing, it’s also incorrect, due to a bug in TThreadedQueue: When you pop the proc from the queue, it does not actually clear the reference inside the queue’s internal list. For unmanaged data types, that’s fine, but for reference-counted types such as interfaces, this causes a memory leak. And if your program is using packages, it gets worse: it can cause a crash at cleanup if the dangling reference came from a package that’s already been unloaded by the time it gets around to finalizing the delayedAction unit.
Thanks to Leif Uneus for the basic idea behind the new, queue-less implementation. Here’s the new version of the unit:
[code lang="Delphi"]
{*****************************************************************************
* The contents of this file are used with permission, subject to
* the Mozilla Public License Version 1.1 (the "License"); you may
* not use this file except in compliance with the License. You may
* obtain a copy of the License at
* http://www.mozilla.org/MPL/MPL-1.1.html
*
* Software distributed under the License is distributed on an
* "AS IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
* implied. See the License for the specific language governing
* rights and limitations under the License.
*
*****************************************************************************
*
* This file was created by Mason Wheeler. He can be reached for support at
* www.turbu-rpg.com.
*****************************************************************************}
unit delayedAction;
interface
uses
SysUtils;
procedure DelayExec(const action: TProc);
implementation
uses
Windows, Messages, Classes, Forms;
type
TDelayQueue = class
private
FHandle: HWND;
procedure MainWndProc(var Msg: TMessage);
public
constructor Create;
destructor Destroy; override;
end;
{ TDelayQueue }
constructor TDelayQueue.Create;
begin
FHandle := AllocateHWnd(self.MainWndProc);
end;
destructor TDelayQueue.Destroy;
begin
DeallocateHWnd(FHandle);
inherited Destroy;
end;
Procedure AnonProcPerform(value: WPARAM);
var
PProc : ^TProc;
Proc : TProc;
begin
PProc := Pointer(value);
Proc := PProc^;
Dispose(PProc);
Proc();
end;
procedure TDelayQueue.MainWndProc(var Msg: TMessage);
begin
try
if Msg.Msg = WM_USER then
AnonProcPerform(msg.WParam)
else
DefWindowProc(FHandle, msg.Msg, msg.WParam, msg.LParam);
except
Application.HandleException(Self);
end;
end;
var
delay: TDelayQueue;
Procedure DelayExec( const action: TProc);
var
PProc: ^TProc;
begin
New(PProc);
PProc^ := action;
PostMessage(delay.FHandle, WM_USER, WPARAM(PProc), 0);
end;
initialization
delay := TDelayQueue.Create;
finalization
delay.Free;
end.
[/code]
“a bug in TThreadedQueue: When you pop the proc from the queue, it does not actually clear the reference inside the queue’s internal list. For unmanaged data types, that’s fine, but for reference-counted types such as interfaces, this causes a memory leak”
Unless I’m missing something (which is possible of course), that isn’t quite true – TThreadedQueue uses a fixed-size internal list (well, ‘fixed’ once the class has been instantiated), and just doesn’t bother clearing an item’s slot in that list when it is popped. There’s no memory leak as such however, because once that slot is taken up again, or the queue object itself is destroyed, the reference count will be decremented.
That said, presumably all that needs doing on EMBT’s part is to assign FQueue[FQueueOffset] to Default(T) once AItem has been assigned. Is it QC’ed…?
Separately, if you want to pass a WPARAM around, then type the value to WPARAM. You’ve tried to outsmart a simple type alias and managed to pick the wrong type… (WPARAM maps to NativeUInt, not NativeInt, at least in newer Delphis, and regardless, the type alias has been around for as long as I can remember.)
A memory leak is memory that stays allocated after you no longer need it. There’s nothing in the definition requiring it to *stay* leaked until shutdown. And yeah, assigning Default should fix this.
Good catch with the WParam. I’ll fix that once I get home.
‘A memory leak is memory that stays allocated after you no longer need it.’
Hmm, I don’t personally give the term ‘memory leak’ quite so broad a definition. When you call FreeMem, for example, the memory typically isn’t deallocated immediately under the hood, as you know, and that’s not a ‘memory leak’ – it’s an optimisation of the Delphi memory manager.
That said, in the context of your original post, talk of a memory leak implied to me a bug in the interaction between generics and anonymous methods (a bug at the compiler or System.pas level in other words), whereas the issue in question was an oversight in the TThreadedQueue code. Plainly, it should still be fixed though…
It’s not so much an issue of memory leaking. The problem is that the object holds a reference when it shouldn’t. That is often crippling.
What you have accomplished is to basically clone what the TThread.Synchronize() and TThread.Queue() methods already do. They both use a thread-safe queue of object methods and anonymous procedures that the main thread executes as needed. So why make your own queuing system instead of using the queuing system already built in to the RTL?
@Remy Read the first article to understand the motivation.
Check out the first article. I wrote this as a replacement for TThread.Queue, because TThread.Queue has a very significant hole in it: it doesn’t work right if you call it from the main thread.
https://quality.embarcadero.com/browse/RSP-15427
Not only they don’t work from other threads, but they have stability issues IME (not sure what the cause, I only looked in the RTL source, spotted the TMonitor usage, assumed perhaps wrongly that the Queue/Synchronize had been written by the same person as the TMonitor and gave up).
I’ve been using a mechanism similar as the one described by Mason since the D7 days, considered dumping it when I saw the additions in Delphi XE, but after a few tests, I went on enhancing my old code rather than try and fix the RTL one. FWIW it’s why the TSynchronizedThreadedDebugger in DWScript isn’t entirely stable, as it relies on TThread.Synchronize, and I haven’t taken the time yet to recreate a working version in Open Source.
This version is certainly better. Nice and simple in general. I was about to make the exact same comment that Chris did. If it’s a WPARAM, then leave it thus until you need to cast.
In fact you made a bit of a meal of AnonProcPerform. No need for taking that extra reference. I’d do it like this:
Procedure AnonProcPerform(P: PProc);
begin
P^();
Dispose(P);
end;
At the call site you do the cast:
AnonProcPerform(PProc(msg.WParam))
I wrote it the way I did so that I could ensure that Dispose would be called even if the proc invocation raises an exception, without needing to put it in a try/finally block.
Fair enough. What’s wrong with writing a try/finally block though? Has the merits of making it clear to the reader what happens in case of exception. And your version has a try/finally too, just that you can’t see it in the code because the compiler puts it in on your behalf.
Much better !
My framework also has overloaded “DelayExec” procedures, like;
Procedure QueueToMainThread( const aProc: TProc); Overload;
Procedure QueueToMainThread( const aProc: TProc; anObject: TObject); Overload;
Procedure QueueToMainThread( const aProc: TProc; anInteger : Integer); Overload;
Procedure QueueToMainThread( const aProc: TProc; aString : String); Overload;
Procedure QueueToMainThread( const anEvent: TNotifyEvent; anObject: TObject); Overload;
using more or less the same principles as shown here.
Ok, the greater than, less than signs and what’s inside got lost in translation, but you get the picture.
TProc(TObject), TProc(Integer) and TProc(String) was the intended type declarations for the middle three procedures.
You should never allocate memory and use that in a Windows message. There are limits in the amount of messages that can be queued (I learned that the hard way from a similar class like this one). When you reach that limit (around 10K by default I believe) It will start to drop messages and your nicely allocated memory is never freed and your delayed proc is not called. Always keep a separate thread safe list and use the message as a hint that there are item(s) in the list.
Hi,
see this (German website use google to translate):
http://wladid.de/delphi/klasse-fur-die-einfache-verwendung-von-verzogernden-methoden/
Axel
There is a minor bug in TDelayQueue.MainWndProc(). When handling unknown messages (like system broadcasts, etc), the result of DefWindowProc() is being ignored. It needs to be assigned to the TMessage.Result field:
msg.Result := DefWindowProc(FHandle, msg.Msg, msg.WParam, msg.LParam);
Regarding delayedAction unit.
What happens if the application uses Application.ProcessMessages? I was in 3 projects where they were using ProcessMessages abbundently (about 700-800 calls to ProcessMessages)?