Page 1 of 1
Suggestion to remove the silencing try-except handlers
Posted: Fri Apr 28, 2017 2:05 pm
by hvassbotn
I see that you tried to address the problem with the exception handling, but I don't think the current solution is very good.
The change you added to OutputDebugMessage does not re-raise the original exception. And it is very confusing behavior of a innocent looking function name. I notice you do call it outside of exception handler, also.
Code: Select all
procedure OutputDebugMessage(const aMessage : string);
const
DEFAULT_LINE = 1;
begin
{$IFDEF DEBUG}
OutputDebugString({$IFDEF DELPHI12_UP}PWideChar{$ELSE}PAnsiChar{$ENDIF}(aMessage + chr(0)));
if (GlobalCEFApp <> nil) and GlobalCEFApp.LibLoaded then
CefLog('CEF4Delphi', DEFAULT_LINE, CEF_LOG_SEVERITY_ERROR, aMessage);
{$ENDIF}
if (GlobalCEFApp <> nil) and GlobalCEFApp.ReRaiseExceptions then
raise Exception.Create(aMessage);
end;
A better solution would be:
Code: Select all
except
on e : exception do
begin
OutputDebugMessage('TCefApplication.CreateInternalApp error: ' + e.Message);
if ReRaiseExceptions then
raise;
end;
end;
The best solution is to remove the try-excepts completely, or at least IFDEF them out:
Code: Select all
{$IFDEF NO_EXCEPTION_SILENCE}
finally
end;
{$ELSE}
except
on e : exception do
OutputDebugMessage('TCefApplication.MultiExeProcessing error: ' + e.Message);
end;
{$ENDIF}
Re: Suggestion to remove the silencing try-except handlers
Posted: Fri Apr 28, 2017 2:51 pm
by salvadordf
I'll take a look at this over the weekend.
Re: Suggestion to remove the silencing try-except handlers
Posted: Sun Apr 30, 2017 5:49 pm
by salvadordf
I need to have the try...except blocks for BriskBard and I'm not convinced that adding so many {$IFDEF} blocks would result in a clean code base.
I've created a new function to handle all exceptions, different than OutputDebugMessage.
I've tested it and it raises the exceptions if you set GlobalCEFApp.ReRaiseExceptions to True.
It will be included in the next version of CEF4Delphi.
Re: Suggestion to remove the silencing try-except handlers
Posted: Tue May 02, 2017 7:49 am
by hvassbotn
But why do you need the exception handlers for BriskBard?
What purpose do they serve?
Do you expect exceptions here in normal cases?
If so, it would be better to handle specific exception classes instead of the generic Exception class.
The problem with this, is that you're hiding programming bugs - like access violations etc.
Much better to use an exception logging system like madExcept or similar.
Refrences:
https://www.thoughtco.com/handling-exce ... ng-1058215
Note that the else part would grab all (other) exceptions, including those you know nothing about. In general, your code should handle only exceptions you actually know how to handle and expect to be thrown.
Also, you should never "eat" an exception
https://pascal.today/2016/12/15/try-or-not-to-try/
Try to never suppress the exceptions with the empty except end clause. This is just bad behavior!
Re: Suggestion to remove the silencing try-except handlers
Posted: Tue May 02, 2017 9:27 am
by salvadordf
This is getting a philosophical discussion about coding styles
I prefer to show customized error messages for some applications instead of raw error messages.
Anyway, please try the new CEF4Delphi version and set GlobalCEFApp.ReRaiseExceptions := True to get all missing exceptions.
If you still need to remove a given try..except block just let me know what function/procedure and I'll set it as virtual.
Notice that I moved all private declarations to the protected section because I assumed some people would like to have full access in their inherited classes.
Re: Suggestion to remove the silencing try-except handlers
Posted: Tue May 02, 2017 11:58 am
by hvassbotn
> I prefer to show customized error messages for some applications instead of raw error messages.
Maybe. But then that should be done in the application level, not in a general component meant to be reused in many different kinds of applications.
Re: Suggestion to remove the silencing try-except handlers
Posted: Wed May 03, 2017 3:09 pm
by hvassbotn
I'm looking into the latest version of CEF4Delphi now. I'm still not happy with the exception handling.
It's now:
Code: Select all
procedure CustomExceptionHandler(const aMessage : string);
begin
OutputDebugMessage(aMessage);
if (GlobalCEFApp <> nil) and GlobalCEFApp.ReRaiseExceptions then
raise Exception.Create(aMessage);
end;
try
// Do actual work
except
on e : exception do
CustomExceptionHandler('TCefApplication.CreateInternalApp error: ' + e.Message);
end;
Problems with this:
- Original exception type and any extra Exception property values is lost
- Original stack trace is lost
- OutputDebugMessage is always called for exceptions
The proper way of re-raising exceptions can be read about here:
http://docwiki.embarcadero.com/RADStudi ... Exceptions
http://stackoverflow.com/questions/2923 ... logging-it
If you still insist on having this unnecessary exception handling in all applications that uses the library, at least do it properly.
My suggestion is:
Code: Select all
function CustomHandleException(const Source: string; E: Exception): boolean;
begin
if Assigned(GlobalCEFApp) and GlobalCEFApp.LogExceptionsToOutputDebugMessage then
OutputDebugMessage(Source + E.Message);
Result := not Assigned(GlobalCEFApp) or GlobalCEFApp.ReRaiseExceptions;
end;
try
// Do actual work
except
on E: Exception do
if not CustomHandleException('TCefApplication.CreateInternalApp error: ', E) then
raise;
end;
This requires a new boolean LogExceptionsToOutputDebugMessage property on the TCefApplication class.
/H
Re: Suggestion to remove the silencing try-except handlers
Posted: Thu May 04, 2017 7:51 am
by salvadordf
I'll take a look as soon as I have time.