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]

 

17 Comments

  1. “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…

    • David Heffernan says:

      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.

  2. Remy Lebeau says:

    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?

  3. David Heffernan says:

    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))

    • Mason Wheeler says:

      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.

      • David Heffernan says:

        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.

  4. Leif Uneus says:

    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.

    • Leif Uneus says:

      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.

  5. Ritsaert Hornstra says:

    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.

  6. Remy Lebeau says:

    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);

Leave a Reply