Page 1 of 1

Message pump in HandleException() causes unwanted recursion

Posted: Wed Oct 05, 2016 2:35 pm
by anders
I've just discovered that there's a message pump in the HandleException() function.
This is really unfortunate as it has caused quite a few errors in one of our applications.

We have registered a "hidden" exception handler using RegisterHiddenExceptionHandler() in order to log handled exceptions.
The list of these exceptions is included in our bug reports.

The scenario that causes the problem goes something like this:
  1. The user clicks a button on a form.
  2. As a response to this we need to execute some code XXX and only then refresh the user interface.
  3. We post a message to ourselves using PostMessage().
    The message handler for this message will update the user interface.
  4. During execution of the XXX code an exception is raised and handled.
  5. MadExcept intercepts the destruction of the exception object and calls HandleException().
  6. HandleException() pumps the message queue.
  7. Our message is fetched from the queue and dispatched.
  8. The message handler updates the user interface prematurely -> Error.
Since it is a quite common technique to use messages to defer execution I find it very problematic that HandleException() pumps the message queue.

Why does it do that anyway?
Since I register the hidden exception handler with stDontSync no messages should be involved and even if MadExcept uses messages internally it should limit the message pump to its own messages.

Re: Message pump in HandleException() causes unwanted recurs

Posted: Mon Oct 10, 2016 8:01 am
by madshi
I understand the problem, but I'm between a rock and a hard place here. If I don't handle messages, your application's VCL forms will all stop responding and the app will look totally frozen/non-responsive (except for the exception box). That gives users a very bad feeling about stability.

IMHO the ideal design of a VCL GUI application should have the main thread do nothing but GUI updates. All serious work should be done in secondary threads, which should finally send a message to the main thread to update the GUI. This way the GUI would always stay perfectly reponsive, no matter how long the "serious work" takes, and it wouldn't ever harm to process messages in any place in the main thread. But I suppose I'm living a pipe dream here. Probably no real world GUI apps are built this way... :(

For most madExcept users the current default behaviour doesn't seem to be a problem. But I suppose I could add an option to disable message handling in the main thread. FWIW, message handling is already disabled in all secondary threads, it's only active in the main thread, and it's done intentionally. But adding an option should not be a big deal.

What do you think?

Re: Message pump in HandleException() causes unwanted recurs

Posted: Mon Oct 10, 2016 1:27 pm
by anders
I'm afraid my position is that neither the design of the VCL or my application is at fault here.

Your justification for the message loop isn't really relevant to the problem I'm reporting (hidden exception handler), but I'll address the use of the message loop anyway and get back to the original problem at the end.

I understand what you're saying about keeping the UI updated and responsive but I disagree that it is important in this context. When MadExcept handles an exception it is supposedly because an error has occurred. Handling and reporting this error should be the primary concern - it is after all the sole reason why we use MadExcept. The application has already crashed. Making the crash "look good" to the user should be a remote secondary priority.

What you're doing corresponds to calling Appliccation.ProcessMessages - something that, I'm sure you know, one should never do - for any reason.

If the purpose of your message pump is to process internal MadExcept messages then you should replace GetMessage() with PeekMessage() and limit it to only retrieve those messages.

If the purpose is to have the main UI repaint itself then you should should replace GetMessage() with PeekMessage() and limit it to only retrieve WM_PAINT messages.
However, processing WM_PAINT messages in an exception handler opens up for an exception flood if the error being handled is caused by the application UI.

My guess is that the purpose of the message pump in HandleException() is to process messages for the main thread UI while the bug report dialog is visible. What you have done is similar to a modal message loop except that the bug report dialog isn't modal (which then opens up for recursion).
In my case I would definitely prefer a modal dialog instead. I do not want my users to continue doing anything with the application until they have taken action on the error being presented to them.

As far as I remember the MadExcept error dialog is executed in a separate thread.
In theory this is sound since the main thread might have be been so corrupted that it cannot handle displaying a dialog. However your message loop in HandleException() completely defeats that safeguard. Since you are fetching and dispatching all messages from the main thread while the bug report dialog is visible, you could just as well have the main thread handle the bug report dialog.

Back to the original problem; In the particular case where I'm handling a "hidden exception" (I.e. an exception that has already been handled by my application) I do not, under any circumstances, want the message queue to be pumped for any other messages than what ever MadExcept itself has put on it. Since I'm just logging the fact that an exception was thrown, caught and handled, there's no reason for any of MadExcept's UI code to be involved. I just want my event handler to be called. Nothing more.
IMO while an option might fix this, the fact that the message pump code is at all involved when there's no reason for it to be, is a sign of a problem in the design of HandleException().

Re: Message pump in HandleException() causes unwanted recurs

Posted: Wed Oct 12, 2016 11:09 am
by madshi
I didn't say your application or the design of the VCL was at fault. I was just saying that with an application that is designed in a more optimal way the problem would not occur, but I also said that probably only few (if any) applications would be designed that way.

madExcept itself does not need to do message handling in this specific situation, but as I explained earlier, I'm doing it intentionally for the one and only purpose of making sure that the application itself (not madExcept) does not appear to be totally frozen to the end user.

I understand your point of view. But please understand that in more than 15 years of madExcept, you're the first user who complains about this specific problem. It's quite possible that some users ran into this problem over the last 15 years and just accepted or ignored it, but nobody else complained about it yet, I think that does show that it's not a problem many users appear to be seeing.

Now of course I could just change madExcept's design by dropping message handling in that situation, but I'm 99.9% sure that I would get many many complaints from madExcept users, because they won't like that all the application windows suddenly appear to be frozen, which was never the case before.

It's true that when there was a serious crash, and the main thread is unstable, doing message handling in the main thread can cause follow-up crashes, so it's not ideal. But on the other hand, there are many many much less serious exceptions, like the user entering a string in a number-only edit field, or a failure to open a file or things like that. In such situations the application should not stop being responsive! Unfortunately it's hard to say which crashes are serious and which are not (except maybe for AVs).

Anyway, as I suggested, I could add an option for this. But I don't think right now that I want to change the default behaviour, because I would get many many user complaints if I did.

Re: Message pump in HandleException() causes unwanted recurs

Posted: Fri Mar 10, 2017 4:11 pm
by madshi
Sorry for taking so long. The next build will have this variable, exported by madExcept.pas:

Code: Select all

var HandleMessagesInMainThread : boolean = true;   // do we want messages to be handled in the main thread, while waiting for the user to close the exception box?

Re: Message pump in HandleException() causes unwanted recurs

Posted: Fri Mar 17, 2017 2:12 pm
by anders
madshi wrote:...But on the other hand, there are many many much less serious exceptions, like the user entering a string in a number-only edit field, or a failure to open a file or things like that. In such situations the application should not stop being responsive!
Less serious exceptions, such as invalid input or failure to open a file, should not propagate to MadExcept and be presented to the user by MadExcept. They should be handled by the application and, if applicable, be presented to the user in a more suitable way.
madshi wrote:Unfortunately it's hard to say which crashes are serious and which are not (except maybe for AVs).
Not really. A handled exception is not serious. An unhandled exception is a bug and is serious. The purpose of MadExcept is to process the unhandled, i.e. serious, exceptions.

Anyhow, the HandleMessagesInMainThread option might help against exceptions floods caused by the message pump, but regardless of that I still can't see any justification for pumping messages when the "hidden exception" handler is invoked.

Re: Message pump in HandleException() causes unwanted recurs

Posted: Tue Mar 21, 2017 9:32 pm
by anders
Oh, and by the way:
madshi wrote:I understand your point of view. But please understand that in more than 15 years of madExcept, you're the first user who complains about this specific problem.
Not quite:
HiddenExceptionHandler calling DispatchMessage