Best way of handling Exceptions

[Berocoder 27]

Today I had a debate with ChatGPT how to handle Exceptions in Delphi 🙂
First it try to talk about Java but I remind that now I am using Delphi...
I don't post the whole conversation as it is boring to read.

My convention is to do something like this:

    try
      sqhpartyroles.sql := fsqlhandle.buildsql;
      sqhpartyroles.executesql;
    except
      on e: exception do
      begin
        e.message := format('error %s from sql %s', [e.message, sqhpartyroles.sql]);
        raise;
      end;
    end;

    
If an exception happen I catch it in a global exceptionhandler to log callstack and exceptions message.
In this case I also want to add some more information in the message.

    
Reply of ChatGPT:
In your scenario, you are indeed preserving the original error message. However, modifying the message of an already thrown exception is generally considered a bad practice, for a few reasons:

1. Reusability and encapsulation: If some other part of the code also catches and handles the same exception, it might receive an error message that has been modified somewhere else in the code, which can be quite confusing.

2. Concurrency issues: In a multithreaded environment, exception objects might be shared among multiple threads. If one thread modifies an exception message, it affects all the other threads that might be handling the same exception.

3. Traceability: By altering the exception message, you lose line numbers and other stack trace information that can help locate where the exception initially occurred.

Instead of modifying the original exception, you can create and throw a new exception, adding your custom message while retaining the precious stack trace of the original exception. 

In Delphi, the way to do this would be to raise a new exception and pass the original exception as an inner exception:

try
  sqhpartyroles.sql := fsqlhandle.buildsql;
  sqhpartyroles.executesql;
except
  on e: exception do
  begin
    raise Exception.CreateFmt('error %s from sql %s', [e.message, sqhpartyroles.sql]).SetInnerException(e); 
  end;
end;

 

In this way you have a new exception with the additional context (the SQL that caused the problem), but you also keep the original exception with all its crucial debugging details. However, please note that `SetInnerException` is not a standard Delphi RTL method. To use it, you need to extend the `Exception` class and add it by yourself:

type
  TMyException = class(Exception)
  private 
    FInnerException: Exception;
  public
    function SetInnerException(e: Exception): TMyException;
    property InnerException: Exception read FInnerException;
  end;
  
function TMyException.SetInnerException(e: Exception): TMyException;
begin
  FInnerException := e;
  Result := Self;
end;

 

I have seen some references to InnerException but never used it.
What is your best practice for exceptions ?

Edited yesterday at 03:29 PM by Berocoder

[David Heffernan 2406]

Handle them as little as possible is the rule you need. Only mutate the exception if you need to. 

[Remy Lebeau 1552]

I (relunctantly) agree with ChatGPT on this one - you should not modify the original exception, but instead you should raise a new exception that captures the original exception into the new exception's InnerException property.

 

However, the way that ChatGPT demonstrates this is wrong:

 

• Firstly, SysUtils.Exception does not have a public SetInnerException() method.  ChatGPT suggests defining a new class, but then doesn't use that class. And that class is just trying to replicate a feature that already exists natively in SysUtils.Exception.
• Second, ChatGPT's approach does not release ownership of the original Exception, so the try..except will still try to free it when the except block exits, leaving the new Exception holding a dangling InnerException pointer to a now-dead Exception.

 

The correct way to capture an InnerException is to use the SysUtils.Exception.RaiseOuterException() method, eg:

try
  sqhpartyroles.sql := fsqlhandle.buildsql;
  sqhpartyroles.executesql;
except
  Exception.RaiseOuterException(Exception.CreateFmt('error from sql %s', [sqhpartyroles.sql])); 
end;

(I don't like this syntax, but it is what it is...)

 

Higher up the call chain, you can then catch the new Exception and traverse its InnerException chain if you want to report/log all of the individual error messages, eg

procedure MyForm.Application1Exception(Sender: TObject; E: Exception);
begin
  repeat
    // use E as needed, then...
    E := E.InnerException;
  until E = nil;
end;

 

 

Edited yesterday at 06:54 PM by Remy Lebeau

[Berocoder 27]

Thanks for the code. I work on rather big application with centralized handling of exceptions.
So in most cases try/except is not used.
But there is also many places where we want a more customized behaviour.
Like add more information to the exceptionmessage.
Callstack is generated with JCL. But we have also tried EurekaLog.

It is VERY important that the linenumber of original exception is not lost when log callstack.
So that is why I am suspicious at ChatGpts suggestion. It also look more complicated that just modify the Errormessage.

But it would be nice to find a more complete demo of how RaiseOuterException is used and how global exceptionHandler can traverse inner exceptions.
https://docwiki.embarcadero.com/Libraries/Athens/en/System.SysUtils.Exception.RaiseOuterException don't have much information.
Another thing. Raise Exceptions is slow. So raise one Exception and then raise again should be twice as slow ?

Edited yesterday at 06:46 PM by Berocoder

[Remy Lebeau 1552]

12 minutes ago, Berocoder said:

It is VERY important that the linenumber of original exception is not lost when log callstack.
So that is why I am suspicious at ChatGpts suggestion. It also look more complicated that just modify the Errormessage.

You should be able to do that, as each Exception object in the chain is preserved as-is and would carry its own CallStack, which JCL can populate as each Exception is raised.

12 minutes ago, Berocoder said:

But it would be nice to find a more complete demo of how RaiseOuterException is used and how global exceptionHandler can traverse inner exceptions.
https://docwiki.embarcadero.com/Libraries/Athens/en/System.SysUtils.Exception.RaiseOuterException don't have much information.

https://blog.eurekalog.com/2010/05/new-exception-class-in-delphi-2009-and_05.html

12 minutes ago, Berocoder said:

Another thing. Raise Exceptions is slow. So raise one Exception and then raise again should be twice as slow ?

Not really, because you are still raising twice. Whether you raise and then re-raise 1 object, or raise 2 objects, is up to you.

[Kas Ob. 128]

I have thoughts on this subject and this code and its approach, specially about raise in except..end clause or re-raising,

 

1) OS(s) are evolving, like Windows, it is getting harsher by version against exceptions in particular, specially the ones that require long unwind, that code could raise and take the application with it, it is portability is question too, might work just perfectly in production then you refactor that code now the "raise" unwinding involve a trip to OS then back at least once and the OS with its new protection (CFG, SafeSEH...) could trigger killing the application in rare moment and leaving non useful report.

 

2) Your code is assuming the exception is solemnly is coming from "sqhpartyroles", so what if the exception has different origin like page fault or zero division, your raising and message just obfuscating the whole thing and confusing the error message and the logs.

 

3) Inside the except..end you are assuming sqhpartyroles is still nice and accessible, what happen when sqhpartyroles.sql is toast, an unintended exception will be raised from here in, and sending you in witch hunt.

 

4) Delphi is object oriented, and it is all about encapsulation, while re-raise or catch and raise is more like leftover form past era or functional programming, exception are not for signaling failure (passing through) or passing data.

 

"raise;" is not dangerous per se, but not much useful too, it is a short cut (may be ), i would suggest to redesign you exception handling, centralized or not, 

You could either 

1) Extended these objects you doubt that can raise exception, into they catch their own exception and pass them to newly introduced object/class as handler for exception, as example extend your TSQLQuery and add a property for TMyAppExceptionHandler...

2) centralize the SQL execution/opening into special object that have in the parameter everything from the TSqlQuery, TTable.... then open or execute, and it will handle the execution and exceptions there, in-place.

3) (2) could be a function instead of class/object, i like it thread safe and globally accessible.

4) The code responsible for performing the SQL operation will call the exception handler with exception object and its parameter (which could be the parameter for itself), then that code is also protected/shielded from raising exception inside try..except, thus you have solid exception handling, even that second try..except is nested one to make sure even when handling is went in worst case scenario then at least a log with "fatal exception in handling SQL exception at xxxxx".

[Berocoder 27]

The code I wrote was just an example. Actually the project don't have much code to handle raw SQL. We have a framework for that. It is also very big project. About 9 MLOC when do a full build.
So the goal is always to do as much as possible in the global exception handler. I cannot evaluate all methods if there is a chance for exception or not. There are many thousands of them.
But in some cases there is a need to add more information to the exception message to make it easier to find the reason for exception and fix it later.
This is the main reason to do try/except in code

Edited 12 hours ago by Berocoder

[Kas Ob. 128]

49 minutes ago, Berocoder said:

But in some cases there is a need to add more information to the exception message to make it easier to find the reason for exception and fix it later.
This is the main reason to do try/except in code

That is perfect and the right way, but don't validate e:exception or any parameter inside except..end; just ship them somewhere that already has its own try..except, thus safely can be handled and logged (by handled i mean the exception itself and its log), inside the original you can use a boolean to exit gracefully locally (that also can be returned as parameter or as a result) instead of depend on "raise" to exit the branch the code in, start with these places you mentioned that need log expanding.

[David Heffernan 2406]

2 hours ago, Kas Ob. said:

inside the original you can use a boolean to exit gracefully locally (that also can be returned as parameter or as a result)

Why bother with exceptions then at all? This is just back to error handling without exceptions where every single function call returns a status code or flag, like calling a Win32 function. 

[Kas Ob. 128]

1 hour ago, David Heffernan said:

Why bother with exceptions then at all? This is just back to error handling without exceptions where every single function call returns a status code or flag, like calling a Win32 function. 

That would the perfect code and most safest one.

 

Raising exception for error, any error, that is wrong in my opinion, take file handling, yes we are in era that files and drives are mounted and removable, so why raising exception, it is error or just failure, exception should indicate critical problem not simple operation that can happen any time and should be expected to begin with, like lost TCP connection why the exception it is error (or failure) and happen all the time, it is expected, so handle it gracefully and try to re-connect or something, but not violate the whole logic flow using raising exception, same goes with DB operations, lost connection to server or failed SQL, while lost server connection is or should be expected and not exception, bad SQL could be raising exception to prevent data corruption, then halt the whole if possible or just log it to be prevented in future,.

 

Losing server connection can't be prevented by code or its developer, and that is the difference between error and exception, unlike bad/broken SQL, an error here means the code is missing edge cases or sanitizing the data or...., well in my opinion.

 

I don't want to steer this thread far form OP topic about specific structure argument with AI, but exceptions in my opinion is not far from using GOTO !, well one can say it is very similar trapped/targeted GOTO, is GOTO illegal ? no , it is unavoidable in some cases, but is it recommended ? no.

[David Heffernan 2406]

OK so your view is not to raise exceptions at all, in which case Delphi is the wrong language for you because you can't avoid dealing with them. But this question is based on the premise that exceptions are used so responding with this viewpoint seems a little off topic. 

[Kas Ob. 128]

Not at all, specially Delphi RTL is plagued with exceptions, something to live with, but don't add more, and don't depend on re-raise, when possible handle in place or encapsulate these handlers then delegate if needed.

 

In other words, just don't complicate the exception thing, personal view after all, like for me choosing between library that raise exception and one handle failure gracefully with event, also when designing a interface/class then an event for failure will relief any one will use it from worrying about exception.

 

And yes this is way off topic, and sorry for that.