6

My problem is that if a thread Posting messages rapidly to the main UI thread and if I update the UI at that time, sometimes the main message queue get stuck (I have no better words to describe this).

Here is the simplified repro code:

const
  TH_MESSAGE = WM_USER + 1; // Thread message
  TH_PARAM_ACTION = 1;
  TH_PARAM_FINISH = 2;

type
  TForm1 = class(TForm)
    Button1: TButton;
    Label1: TLabel;
    procedure Button1Click(Sender: TObject);
  private
    ThreadHandle: Integer;
    procedure ThreadMessage(var Message: TMessage); message TH_MESSAGE;
  public
  end;

var
  Form1: TForm1;

implementation

{$R *.dfm}

function ThreadProc(Parameter: Pointer): Integer;
var
  ReceiverWnd: HWND;
  I: Integer;
  Counter: Integer;
begin
  Result := 0;
  ReceiverWnd := Form1.Handle;
  Counter := 100000;
  for I := 1 to Counter do
  begin
    PostMessage(ReceiverWnd, TH_MESSAGE, TH_PARAM_ACTION, I);
    //Sleep(1); // <- is this the cure?
  end;
  PostMessage(ReceiverWnd, TH_MESSAGE, TH_PARAM_FINISH, GetCurrentThreadID);
  OutputDebugString('Thread Finish OK!'); // <- I see this
  EndThread(0);
end;

procedure TForm1.ThreadMessage(var Message: TMessage);
begin
  case Message.WParam of
    TH_PARAM_ACTION:
      begin
        Label1.Caption := 'Action' + IntToStr(Message.LParam);
        //Label1.Update;
      end;
     TH_PARAM_FINISH:
       begin
         OutputDebugString('ThreadMessage Finish'); // <- Dose not see this
         Button1.Enabled := True;
         CloseHandle(ThreadHandle);
       end;
  end;    
end;

procedure TForm1.Button1Click(Sender: TObject);
var
  ThreadId: LongWord;
begin
  Button1.Enabled := False;
  ThreadId := 1;
  ThreadHandle := BeginThread(nil, 0, @ThreadProc, nil, 0, ThreadId);
end;

I do realize that the worker thread loop is very busy. I thought that since the thread is Posting messages to the main UI thread, it (the main UI thread) has the chance to process it's messages while receiving other messages from the worker thread.
The problem escalates as I increase the counter.

Problems:
I never see Label1 being updated unless I add Label1.Update; and the main UI is blocked.
TH_PARAM_ACTION never reaches 100000 (in my case) - randomly above 90000.
TH_PARAM_FINISH never gets to the message queue.
Obviously the CPU usage is very high.

Questions:
What is the correct way to handle this situation? Are messages posted from the worker thread being removed from the message queue (if yes, then why)?
Is Sleep(1) in the loop is really the cure to this problem? if yes then why 1? (0 does not)


OK. Thanks to @Sertac and @LU I now realize that the message queue has a limit, and now checking for result from PostMessage with ERROR_NOT_ENOUGH_QUOTA.but, still the main UI is NOT responsive!

function ThreadProc(Parameter: Pointer): Integer;
var
  ReceiverWnd: HWND;
  I: Integer;
  Counter: Integer;
  LastError: Integer;
  ReturnValue, Retry: Boolean;
begin
  Result := 0;
  ReceiverWnd := Form1.Handle;
  Counter := 100000;
  for I := 1 to Counter do
  begin
    repeat
      ReturnValue := PostMessage(ReceiverWnd, TH_MESSAGE, TH_PARAM_ACTION, I);
      LastError := GetLastError;
      Retry := (not ReturnValue) and (LastError = ERROR_NOT_ENOUGH_QUOTA);
      if Retry then
      begin
        Sleep(100); // Sleep(1) is not enoght!!!
      end;
    until not Retry;
  end;
  PostMessage(ReceiverWnd, TH_MESSAGE, TH_PARAM_FINISH, GetCurrentThreadID);
  OutputDebugString('Thread Finish OK!'); // <- I see this
  EndThread(0);
end;

Just for reference here is the original code I was inspecting:
Delphi threading by example

This example searches text in files (5 threads simultaneously). Obviously when you make a task like that, you must see all the matching results (in a ListView for example).

Problem was that if I searched in meany files, and the search string was short (like "a") - there were meany matches found. the busy loop while FileStream.Read(Ch,1)= 1 do was posting messages rapidly (TH_FOUND) with the match and flooding the message queue.

Messages where in fact not getting to the message queue. as @Sertac mentioned "a message queue has a limit of 10000 by default".

From MSDN PostMessage

There is a limit of 10,000 posted messages per message queue. This limit should be sufficiently large. If your application exceeds the limit, it should be redesigned to avoid consuming so many system resources. To adjust this limit, modify the following registry key (USERPostMessageLimit)

As others said, this code/pattern should be redesigned.

kobik
  • 21,001
  • 4
  • 61
  • 121
  • I'm not sure of exact reasoning, but I've been told using `Sleep` in a thread is too risky. – Jerry Dodge Sep 20 '14 at 12:26
  • 2
    *"Are messages posted from the worker thread being removed from the message queue (if yes, then why)?*" Yeah, a message queue has a limit of 10000 by default (search for USERPostMessageLimit). – Sertac Akyuz Sep 20 '14 at 12:26
  • @Sertac, Ahhhhh! I had the "feeling" that the queue has a limit (never looked it up). so you think it is safe to do something like `if i mod 10000 = 0 then PostMessage`... is this the correct pattern? (but then I loose the info I want in my main thread :/ ) – kobik Sep 20 '14 at 12:32
  • 3
    Better to check the result of the PostMessage, or put the message on a queue of your own. – LU RD Sep 20 '14 at 12:35
  • Instead of flooding the main thread/ui with messages from a thread, you should first lookup if a previous not yet processed message is now obsolete. Use a second thread for handling this messages within a queue and use `SendMessage` to inform the main thread. If a new message from the first thread arrives, check for obsolete messages in the queue and remove them. – Sir Rufo Sep 20 '14 at 12:36
  • @kobik - I think 'Update' is needed on the label. Even if you have a 'mod', a WM_PAINT will never be processed as long as there's another posted message in the queue. – Sertac Akyuz Sep 20 '14 at 12:39
  • 4
    FYI Normal users can recognize 25 images per second. If the ui changes faster than this, some of the information will never reach the user ;o) – Sir Rufo Sep 20 '14 at 12:48
  • 1
    That's probably why Sleep(0) is not enough, which only yields. Sleep(1) is effectively Sleep(10) (clock resolution) which is probably enough for the main thread to empty the queue and then process the pending paint message. I guess I'd use a Sleep together with 'mod' in the end. – Sertac Akyuz Sep 20 '14 at 12:48
  • @SertacAkyuz Instead of throttle the thread you should throttle the ui messages – Sir Rufo Sep 20 '14 at 12:50
  • @Sir - It would depend what you're doing. For some kind of user feedback, I would think it would be over engineering. – Sertac Akyuz Sep 20 '14 at 12:52
  • 1
    @SertacAkyuz If the "feedback" is faster than the user can recognize, then it is no feedback. And it is no over engineering to keep in mind the limits of the system and the user. – Sir Rufo Sep 20 '14 at 12:57
  • @Sir - You can have some elapsed time checking in every 'mod' for that concern. – Sertac Akyuz Sep 20 '14 at 12:59
  • Regarding your edit, you need a complete rethink. If the queue is ever full, it's game over. Sleep is no good. You should read my answer and do what I suggest in the final paragraph. – David Heffernan Sep 20 '14 at 18:11
  • @Sir, *"Use a second thread for handling this messages within a queue"* Do you mean that the first thread should post messages to the second thread? – kobik Sep 21 '14 at 21:43
  • No, just add the message to the thread (CriticalSection needed) and check for old now obsolete messages for the UI and remove them. – Sir Rufo Sep 21 '14 at 22:53

2 Answers2

8

You are flooding the message queue at a rate greater than the rate at which the messages are being processed. Eventually the queue becomes full.

If you absolutely need every single message to be handled by the main thread you'll need to maintain your own queue. And you'll likely need to throttle the thread that adds to the queue.

Your Sleep(1) will throttle, but in a very crude way. Perhaps it will throttle too much, perhaps not enough. In general you'd need to be more precise about throttling. Typically you'd throttle adaptively by keeping track of the size of the queue. If you can avoid throttling do so. It's complex, hard to implement well, and hurts performance.

The call Sleep(0) will yield if there's another thread ready to run. Otherwise Sleep(0) has no effect. From the documentation

A value of zero causes the thread to relinquish the remainder of its time slice to any other thread that is ready to run. If there are no other threads ready to run, the function returns immediately, and the thread continues execution.

On the other hand, if all you need to do is report status in your GUI then you should avoid a queue altogether. Don't post messages from the thread to the main thread. Simply run a GUI update timer in your main thread and have the main thread ask the workers for their current status.

Applying that idea to your code results in this:

const
  TH_MESSAGE = WM_USER + 1; // Thread message
  TH_PARAM_FINISH = 2;

type
  TForm1 = class(TForm)
    Button1: TButton;
    Label1: TLabel;
    Timer1: TTimer;
    procedure Button1Click(Sender: TObject);
    procedure Timer1Timer(Sender: TObject);
  private
    procedure ThreadMessage(var Message: TMessage); message TH_MESSAGE;
  end;

var
  Form1: TForm1;

implementation

{$R *.dfm}

var
  Count: Integer;

function ThreadProc(Parameter: Pointer): Integer;
var
  ReceiverWnd: HWND;
  I: Integer;
begin
  Result := 0;
  ReceiverWnd := Form1.Handle;
  for I := 1 to high(Integer) do
  begin
    Count := I;
  end;
  PostMessage(ReceiverWnd, TH_MESSAGE, TH_PARAM_FINISH, GetCurrentThreadID);
end;

procedure TForm1.ThreadMessage(var Message: TMessage);
begin
  case Message.WParam of
  TH_PARAM_FINISH:
    begin
      Button1.Enabled := True;
      Timer1.Enabled := False;
    end;
  end;
end;

procedure TForm1.Timer1Timer(Sender: TObject);
begin
  Label1.Caption := 'Action' + IntToStr(Count);
end;

procedure TForm1.Button1Click(Sender: TObject);
var
  ThreadId: LongWord;
  ThreadHandle: THandle;
begin
  Count := -1;
  Button1.Enabled := False;
  ThreadHandle := BeginThread(nil, 0, @ThreadProc, nil, 0, ThreadId);
  CloseHandle(ThreadHandle);
  Timer1.Enabled := True;
end;
David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • OK, I now see your point clearly (I Think). It is more complicated than what I was hoping for. I was trying to avoid the main thread from asking the workers for their current status, specially if there are a few workers involved simultaneously. but maybe this is the only pattern that is reliable. BTW, why do you close the `ThreadHandle` right away? – kobik Sep 21 '14 at 07:25
  • 1
    You don't do anything with the handle so you may as well close it. In fact, it should be a local. – David Heffernan Sep 21 '14 at 07:29
  • I didn't see `TThread` doing anything with it's `FHandle` but still the handle was closed in `TThread.Destroy`. and this is why I closed the handle on `TH_PARAM_FINISH`. I guess in `TThread` one *might* do something with the handle... – kobik Sep 21 '14 at 07:46
  • 2
    TThread use the handle in WaitFor. You need a handle to the thread in order to wait for it. The code here doesn't wait and so can close the handle immediately. – David Heffernan Sep 21 '14 at 07:55
3

What is the correct way to handle this situation? Are messages posted from the worker thread being removed from the message queue (if yes, then why)?

Code which can flood the message queue smells and should be redesigned, but if you really need to handle this situation you can check the [boolean] value returned by PostMessage and call GetLastError if PostMessage returned False. GetLastError should return ERROR_NOT_ENOUGH_QUOTA if the message queue is full.

kludg
  • 27,213
  • 5
  • 67
  • 118
  • It's too late by then usually, because other important messages will also be rejected. The only effective way to deal with this is to stop flooing in the first place. – David Heffernan Sep 20 '14 at 16:44
  • So, How to redesign my code? I am now checking result from `PostMessage` and `GetLastError` of `ERROR_NOT_ENOUGH_QUOTA` but the main UI thread is still not responsive. – kobik Sep 20 '14 at 17:38
  • 1
    @kobik, if your aim is showing a few thread states to the user, rather query their states from the main thread periodically (or when the app becomes idle). A simple rule; if your UI thread cannot process such amount of messages on time, stop posting them to it (or decrease their frequency). Or dedicate a class with message queue (you should not target a form handle anyway), which will provide a *snapshot* of all the states to the UI on demand. – TLama Sep 20 '14 at 18:16
  • All you need to do is stop posting messages and add an update timer – David Heffernan Sep 20 '14 at 18:23
  • @TLama, consider this an exercise: the worker thread **must** post every action to the main thread which I **must** log (every action). maybe David is right and the only way for updating the UI will need a timer. anyway, the data must be updated in the log! – kobik Sep 20 '14 at 19:21
  • @kobik, that's what a small class with a message queue (with a window created by the `AllocateHWnd` function) can do. It won't interfere with the UI thread, it will log every processed message and it will have a log queue (FIFO) which the UI thread will be able to consume on demand (which will be done periodically). – TLama Sep 20 '14 at 19:33
  • Update the data in the log then. But keep that separate from the GUI thread status updates. – David Heffernan Sep 20 '14 at 19:33
  • 1
    @TLama that just puts the problem elsewhere. Windows don't have message queues. Threads do. So you need a thread. But if the main thread can't clear the messages quickly enough, why will another thread? – David Heffernan Sep 20 '14 at 19:35
  • @David, I see. You mean I log the data in the worker thread and poll the thread from the UI for UI updates only? What if I want evry action to be noticed in the main thread? – kobik Sep 20 '14 at 19:36
  • @kobik As you've discovered, that is not possible without delaying the thread. If you really want that, use `Synchronize` and block the worker thread whilst you update the GUI. Performance will be terrible, but that's the result of what you are asking for. – David Heffernan Sep 20 '14 at 19:43
  • @David, yes, that puts the problem elsewhere, but the problem is that *"the worker thread must post every action to the main thread"* which I understand as that the main thread must get all the posted messages. And that's where a separate thread writing to a log file and collecting log messages for the UI thread into a queue will be handy. – TLama Sep 20 '14 at 19:49
  • 3
    @kobik, your question is all about updating the GUI, but in comments you mention that you must log every message. I think there are different answers to those specifications. – LU RD Sep 20 '14 at 19:58
  • @kobik logging is a different thing; you can log directly from a background thread; if you log from several threads you need some syncronization (ex critical section). Divide logging logic and UI update. – kludg Sep 21 '14 at 03:51
  • @LURD, the "log" was meant to be an (bad) example. as I wrote "the worker thread **must** post every action to the main thread". in other words, the main UI thread itself must be aware of every action... David's answer gave me the right idea on how to handle this situation, and user246408 (+Sertac and yourself) gave me the reason why *"messages posted from the worker thread being removed from the message queue"* :) – kobik Sep 21 '14 at 07:12
  • @TLama, creating a small class with `AllocateHWnd` and routing messages to it, has the same effect. It's still lives in the main tread. even if not touching main UI, it is still blocked and messages don't go there after a certain point (unless I force retries). – kobik Sep 21 '14 at 21:38