Difference between revisions of "DB Access Exception Handling Policy"

From OpenPetra Wiki
Jump to navigation Jump to search
m (moved Inconsistent DB Access Exception Handling to DB Access Exception Handling: We now have consistent DB Access Exception Handling strategies documented on this page.)
 
(13 intermediate revisions by the same user not shown)
Line 1: Line 1:
 
==Overview==
 
==Overview==
'''We do not yet have a consistent way of handling DB Access Exceptions in all situations. This is strongly desired.'''
+
(''This policy is an extension of the [[Error and Exception Handling Policy]]!'')
  
'''TODO''': Improve section on DB Access Exceptions in the [[Error and Exception Handling Policy#DB_Access | Error and Exception Handling Policy]].
+
'''The correct handling of Exceptions that occur because of database access is very important for the following reasons:'''
 +
# DB Exception Handling '''must ensure''' that
 +
## Any DB Transaction that was started in the same program scope (e.g. C# Method, or a particular 'chain' of C# Method calls) and that was used for the DB command that resulted in an Exception is ''rolled back'' (whether read or write access to the DB occurs). That way a DB transaction can never be left running ('dangling') where it shouldn't. [[Automatic DB Transaction Handling | How to do that!]]
 +
# DB Exceptions '''must not get 'swallowed''''. While suppressing an Exception (by 'swallowing' it) is never a good idea and is [[Error and Exception Handling Policy#'Swallowing'_Exceptions_-_*** DON'T_DO_IT!_*** | actively discouraged]], suppressing a DB Exception is especially bad as the caller of a Method that suppressed the Exception that occurred because of the DB access has no way of knowing of problems that really happened while accessing the DB and hence can not know whether the DB access succeeded, or not.
  
 +
'''Policy: Use the '[[Automatic DB Transaction Handling |Automatic DB Transaction Handling]]' wherever feasible! Only where this is not feasible use the '[[DB Access Exception Handling Policy#Manual_DB_Exception_Handling |Manual DB Exception Handling]]' as a fall-back.'''
  
===Exception Handling===
+
=== The Danger of 'Dangling DB Transactions' ===
* '''Scenario 1''': just reading data
+
DB Transactions that are left running where they shouldn't (sometimes referred to as 'dangling DB Transactions') will very likely cause problems further down the execution path of the program code, or with the next program code path that is executed by the user (esp. after an Exception happened), as other program code...
** '''Way 1''': Commit before first Catch Clause + Commit in Catch Clause(s), no Finally Clause
+
* might attempt to start a new DB Transaction, which will fail as the 'dangling DB Transaction' is still running and only one DB Transaction can be running at a given time;
*** ''Advantage'':
+
* might be written in such a way that it can re-use an existing Transaction, but ...
**** No Finally Clause needed, therfore less deep nesting of code.  
+
** that 'piggybacking on a running DB Transaction' might fail if the Isolation Level that is required by that program code is higher (=more restrictive) than the Isolation Level of the running DB Transaction. Also, that program code could ask for a specific, exact Isolation Level and the the Isolation Level of the running DB Transaction is different. In both cases an Exception is raised (either <code>EDBTransactionIsolationLevelTooLowException</code> or <code>EDBTransactionIsolationLevelWrongException</code>).
*** ''Disadvantage'':
+
** in case the re-using of the DB Transaction doesn't fail when the DB Transaction is requested by that program code then the Isolation Level of the running DB Transaction could be higher than the Isolation Level of the DB Transaction of that program code. That will lead at least to unnecessary DB Locks taken out and can potentially lead to concurrency problems that would otherwise not happen. Both of those issues are ''very hard to pin-point once they happen'' as it is often not obvious what program code has ''initially'' taken out a DB Transaction that then causes the mentioned problems later on when the DB Transaction is re-used (esp. once the application is used by users)!
**** Commit can be accidently forgotten about by developers, leaving an open, 'dangling' DB Transaction - this is also true for the case where a 'return' statement is executed in the 'try' code block of the try-catch Clause!!!
 
** '''Way 2''': Commit in Finally Clause which encloses try-catch Clause
 
*** ''Advantage'': Commit can never be forgotten about - this is also true for the case where a 'return' statement is executed in the 'try' code block of the try-catch-finally Clause!!!
 
*** ''Disadvantage'': Finally Clause is needed, therfore deeper nesting of code.
 
  
'''Recommendation''' (ChristianK): We should discourage 'Way 1' and ensure that 'Way 2' is implemented everywhere.
+
===''Manual'' DB Exception Handling===
 +
==== '''Scenario 1''': Only Reading Data====
 +
While there are several possible solutions to making sure that a DB Transaction that is used for reading gets ended correctly, only the following solution is permitted ''because a Rollback can never be forgotten about'' - this is also true for the case where a 'return' statement is executed in the 'try' code block of the try-(catch-)finally Clause:
  
 +
'''Policy: DB Transaction Rollback ''only'' in Finally Clause which encloses try Clause or try-catch Clause'''.
  
* '''Scenario 2''': Writing Data / mixed reading/writing of data
+
''The DB Transaction must not get Rolled Back before the Finally Clause'' - in other words, the DB Transaction must not get Rolled Back before the first Catch Clause and must also not get Rolled Back in any Catch Claus(es) (if present).
** '''Way 1''': Commit before first Catch Clause + Rollback in Catch Clause(s), no Finally Clause
 
*** ''Advantages'':
 
**** No Finally Clause needed, therfore less deep nesting of code.
 
**** No bool Variable needed (see below).
 
*** ''Disadvantages'':
 
**** Developers must not forget to do a Rollback in ''ANY'' Catch Branch!
 
**** Commit can be accidently forgotten about by developers, leaving an open, 'dangling' DB Transaction.
 
** '''Way 2''': Commit or Rollback in Finally Clause which encloses try-catch Clause, depending on value of bool Variable ('SubmissionOK'). DB Transaction is not committed before first Catch Clause and not Committed / Rolled Back in any Catch Clauses. (Example: <code>C:\openpetraorg\trunk\csharp\ICT\Petra\Server\lib\MSysMan\UserDefaults.cs, line 964</code>)
 
*** ''Advantage'': Commit or Rollback can never be forgotten about - this is also true for the case where a 'return' statement is executed in the 'try' code block of the try-catch-finally Clause!!!
 
*** ''Disadvantages'':
 
**** Finally Clause is needed, therfore deeper nesting of code.
 
**** bool Variable needed.
 
  
'''Recommendation''' (ChristianK): We should discourage 'Way 1' and ensure that 'Way 2' is implemented everywhere.
+
Example: <code>XXX</code>
  
 +
''To Rollback or to Commit?'' There are discussions as to what is the right action to finish a DB Transaction that was used exclusively for reading of data: issue a Rollback or issue a Commit. Some people are in favour of Commiting, but the danger with that approach is that if the DB Transaction that was used for reading was re-used and if in earlier code paths data was written to the DB using that DB Transaction then that data will get committed in the code section that was only reading, and that code section has no knowledge of 'what' it is unknowingly committing to the DB! Some people argue that a Rollback might cause more overhead in the RDBMS than a Commit, but we are still favouring Rollback over Commit because the danger of committing data to the DB unknowingly is not there.
  
===DB Transaction Handling===
+
 
* handling of long running DB Transaction over many Method Calls ('Control Method' / 'Super Structure')
+
''--> New approach that simplifies the implementation of the policy for the software engineers: [[Automatic DB Transaction Handling]] - now ''''policy'''' <--''
** Discussion: http://sourceforge.net/apps/phpbb/openpetraorg/viewtopic.php?p=364#p364
+
 
*** Starting of a local DB Transaction or use of a Session Object?
+
==== '''Scenario 2''': Writing Data / Mix of Reading and Writing of Data ====
*** [[Inconsistent DB Access Exception Handling#Exception Handling | Exception handling]]
+
While there are several possible solutions to making sure that a DB Transaction that is used for writing, or for a mix of reading and writing, gets ended correctly, only the following solution is permitted because a ''DB Transaction Commit or Rollback can never be forgotten about'' - this is also true for the case where a 'return' statement is executed in the 'try' code block of the try-(catch-)finally Clause:
 +
 
 +
'''Policy: DB Transaction Commit or Rollback ''only'' in Finally Clause which encloses try Clause or try-catch Clause.'''
 +
 
 +
''The DB Transaction must not get Committed or Rolled Back before the Finally Clause'' - in other words, the DB Transaction must not get Committed before the first Catch Clause and must also not get Rolled Back in any Catch Claus(es) (if present).
 +
 
 +
Whether a DB Transaction Commit or a DB Transaction Rollback is done in the Finally Clause depends on the value of a boolean Variable ('<code>SubmissionOK</code>') that needs to be declared outside of the try-(catch-)finally block (best done at the beginning of a Method).
 +
* The <code>SubmissionOK</code> Variable ought to be initialised to <code>false</code> in the Variable declaration. The purpose of doing this is twofold:
 +
** The Finally Clause will not throw an Exception on reading the Variable in case the Variable wasn't assigned on purpose before the Finally Clause gets entered;
 +
** It ensures that a DB Transaction Rollback will always occur in case the software engineer forgot to set the Variable to <code>false</code> in an 'error condition'.<br/>  The software engineers are still encouraged to set it to <code>false</code> specifically in an 'error condition' - although this is superfluous, it is clearer that in this case the DB Transaction will be Rolled Back. Put simply, this is the place in program code where one would issue a DB Rollback, and that is substituted with the setting of the <code>SubmissionOK</code> Variable to <code>false</code>.
 +
* The value of that Variable needs to be set to <code>true</code> in a 'success condition' in the flow of the program code that is 'inside' the 'try' code section. This ensures that the DB Transaction will be Committed in the Finally Clause.
 +
 
 +
Example: <code>XXX</code>
 +
 
 +
 
 +
The way how the DB Transaction handling in Scenario 1 and 2 is to be done was made ''''policy'''' upon agreement in the OpenPetra Developers' Conference call on April 29th, 2014.
 +
 
 +
''--> New approach that simplifies the implementation of the policy for the software engineers: [[Automatic DB Transaction Handling]] - now ''''policy''''<--''
 +
 
 +
As of September 2014, it is a reality that quite a lot of DB Transaction handling code in OpenPetra does not meet the policy laid out on this page, nor the [[Automatic DB Transaction Handling]] policy. For that reason ''we agreed that whenever a developer changes anything in a Method that contains a code section that deals with DB Transaction handling code, the developer will change the DB Transaction handling code to the [[Automatic DB Transaction Handling]] approach (wherever feasible; if not then to the policy laid out on this page) in case it doesn't already meet this policy''. That way we the [[Automatic DB Transaction Handling]] policy (wherever feasible; if not then to the policy laid out on this page) will be implemented more and more across the server-side code base.
 +
 
 +
For any ''new server-side code'' that contains DB Transaction handling code the [[Automatic DB Transaction Handling]] policy (wherever feasible; if not then to the policy laid out on this page) is to be implemented ''right from the start'', of course!
 +
 
 +
===DB Transaction Handling When DB Transactions Span Several Methods ===
 +
'''TODO: Write this section once good, solid examples of usage of the [[Automatic DB Transaction Handling]] approach are in trunk''' (e.g. Method 'PostAPDocuments' found in file \csharp\ICT\Petra\Server\lib\MFinance\AP\AP.EditTransaction.cs [which is a 'controlling superstructure'] calling Method 'SaveGLBatchesTDS' found in file \csharp\ICT\Petra\Server\lib\MFinance\GL\GL.Transactions.cs)
 +
 
 +
====Tips for Debugging====
 +
* The OpenPetra <code>TDBTransaction</code> Class (which is exclusively used wherever OpenPetra handles DB Transactions) has got three read-only Properties that can be of help especially when DB Transactions span several Methods. You can inspect those Properties when debugging ('<code>Reused</code>' can be enlightening when inspecting buggy DB Transaction behaviour!) and you could evaluate these Properties in very involved DB Transaction handling code. (Normally, there is no need to use these Properties, though.)
 +
** '<code>Reused</code>': <code>true</code> if the DB Transaction has been re-used ''at least once'' by way of calling one of the '<code>TDataBase.GetNewOrExisting''Auto''Transaction</code>' or '<code>TDataBase.GetNewOrExistingTransaction</code>' Methods. Once this flag got set to '<code>true</code>' it never gets set back to '<code>false</code>'.
 +
*** See also: [https://forum.openpetra.org/t/determining-whether-a-db-transaction-was-started-or-piggy-backed-on/345 Forum Article]
 +
** '<code>Valid</code>': '<code>true</code>' if the DB Transaction hasn't been Committed or Rolled Back, otherwise '<code>false</code>'.
 +
** '<code>IsolationLevel</code>': <code>System.Data.IsolationLevel</code> of the DB Transaction.

Latest revision as of 15:10, 5 February 2015

Overview

(This policy is an extension of the Error and Exception Handling Policy!)

The correct handling of Exceptions that occur because of database access is very important for the following reasons:

  1. DB Exception Handling must ensure that
    1. Any DB Transaction that was started in the same program scope (e.g. C# Method, or a particular 'chain' of C# Method calls) and that was used for the DB command that resulted in an Exception is rolled back (whether read or write access to the DB occurs). That way a DB transaction can never be left running ('dangling') where it shouldn't. How to do that!
  2. DB Exceptions must not get 'swallowed'. While suppressing an Exception (by 'swallowing' it) is never a good idea and is actively discouraged, suppressing a DB Exception is especially bad as the caller of a Method that suppressed the Exception that occurred because of the DB access has no way of knowing of problems that really happened while accessing the DB and hence can not know whether the DB access succeeded, or not.

Policy: Use the 'Automatic DB Transaction Handling' wherever feasible! Only where this is not feasible use the 'Manual DB Exception Handling' as a fall-back.

The Danger of 'Dangling DB Transactions'

DB Transactions that are left running where they shouldn't (sometimes referred to as 'dangling DB Transactions') will very likely cause problems further down the execution path of the program code, or with the next program code path that is executed by the user (esp. after an Exception happened), as other program code...

  • might attempt to start a new DB Transaction, which will fail as the 'dangling DB Transaction' is still running and only one DB Transaction can be running at a given time;
  • might be written in such a way that it can re-use an existing Transaction, but ...
    • that 'piggybacking on a running DB Transaction' might fail if the Isolation Level that is required by that program code is higher (=more restrictive) than the Isolation Level of the running DB Transaction. Also, that program code could ask for a specific, exact Isolation Level and the the Isolation Level of the running DB Transaction is different. In both cases an Exception is raised (either EDBTransactionIsolationLevelTooLowException or EDBTransactionIsolationLevelWrongException).
    • in case the re-using of the DB Transaction doesn't fail when the DB Transaction is requested by that program code then the Isolation Level of the running DB Transaction could be higher than the Isolation Level of the DB Transaction of that program code. That will lead at least to unnecessary DB Locks taken out and can potentially lead to concurrency problems that would otherwise not happen. Both of those issues are very hard to pin-point once they happen as it is often not obvious what program code has initially taken out a DB Transaction that then causes the mentioned problems later on when the DB Transaction is re-used (esp. once the application is used by users)!

Manual DB Exception Handling

Scenario 1: Only Reading Data

While there are several possible solutions to making sure that a DB Transaction that is used for reading gets ended correctly, only the following solution is permitted because a Rollback can never be forgotten about - this is also true for the case where a 'return' statement is executed in the 'try' code block of the try-(catch-)finally Clause:

Policy: DB Transaction Rollback only in Finally Clause which encloses try Clause or try-catch Clause.

The DB Transaction must not get Rolled Back before the Finally Clause - in other words, the DB Transaction must not get Rolled Back before the first Catch Clause and must also not get Rolled Back in any Catch Claus(es) (if present).

Example: XXX

To Rollback or to Commit? There are discussions as to what is the right action to finish a DB Transaction that was used exclusively for reading of data: issue a Rollback or issue a Commit. Some people are in favour of Commiting, but the danger with that approach is that if the DB Transaction that was used for reading was re-used and if in earlier code paths data was written to the DB using that DB Transaction then that data will get committed in the code section that was only reading, and that code section has no knowledge of 'what' it is unknowingly committing to the DB! Some people argue that a Rollback might cause more overhead in the RDBMS than a Commit, but we are still favouring Rollback over Commit because the danger of committing data to the DB unknowingly is not there.


--> New approach that simplifies the implementation of the policy for the software engineers: Automatic DB Transaction Handling - now 'policy' <--

Scenario 2: Writing Data / Mix of Reading and Writing of Data

While there are several possible solutions to making sure that a DB Transaction that is used for writing, or for a mix of reading and writing, gets ended correctly, only the following solution is permitted because a DB Transaction Commit or Rollback can never be forgotten about - this is also true for the case where a 'return' statement is executed in the 'try' code block of the try-(catch-)finally Clause:

Policy: DB Transaction Commit or Rollback only in Finally Clause which encloses try Clause or try-catch Clause.

The DB Transaction must not get Committed or Rolled Back before the Finally Clause - in other words, the DB Transaction must not get Committed before the first Catch Clause and must also not get Rolled Back in any Catch Claus(es) (if present).

Whether a DB Transaction Commit or a DB Transaction Rollback is done in the Finally Clause depends on the value of a boolean Variable ('SubmissionOK') that needs to be declared outside of the try-(catch-)finally block (best done at the beginning of a Method).

  • The SubmissionOK Variable ought to be initialised to false in the Variable declaration. The purpose of doing this is twofold:
    • The Finally Clause will not throw an Exception on reading the Variable in case the Variable wasn't assigned on purpose before the Finally Clause gets entered;
    • It ensures that a DB Transaction Rollback will always occur in case the software engineer forgot to set the Variable to false in an 'error condition'.
      The software engineers are still encouraged to set it to false specifically in an 'error condition' - although this is superfluous, it is clearer that in this case the DB Transaction will be Rolled Back. Put simply, this is the place in program code where one would issue a DB Rollback, and that is substituted with the setting of the SubmissionOK Variable to false.
  • The value of that Variable needs to be set to true in a 'success condition' in the flow of the program code that is 'inside' the 'try' code section. This ensures that the DB Transaction will be Committed in the Finally Clause.

Example: XXX


The way how the DB Transaction handling in Scenario 1 and 2 is to be done was made 'policy' upon agreement in the OpenPetra Developers' Conference call on April 29th, 2014.

--> New approach that simplifies the implementation of the policy for the software engineers: Automatic DB Transaction Handling - now 'policy'<--

As of September 2014, it is a reality that quite a lot of DB Transaction handling code in OpenPetra does not meet the policy laid out on this page, nor the Automatic DB Transaction Handling policy. For that reason we agreed that whenever a developer changes anything in a Method that contains a code section that deals with DB Transaction handling code, the developer will change the DB Transaction handling code to the Automatic DB Transaction Handling approach (wherever feasible; if not then to the policy laid out on this page) in case it doesn't already meet this policy. That way we the Automatic DB Transaction Handling policy (wherever feasible; if not then to the policy laid out on this page) will be implemented more and more across the server-side code base.

For any new server-side code that contains DB Transaction handling code the Automatic DB Transaction Handling policy (wherever feasible; if not then to the policy laid out on this page) is to be implemented right from the start, of course!

DB Transaction Handling When DB Transactions Span Several Methods

TODO: Write this section once good, solid examples of usage of the Automatic DB Transaction Handling approach are in trunk (e.g. Method 'PostAPDocuments' found in file \csharp\ICT\Petra\Server\lib\MFinance\AP\AP.EditTransaction.cs [which is a 'controlling superstructure'] calling Method 'SaveGLBatchesTDS' found in file \csharp\ICT\Petra\Server\lib\MFinance\GL\GL.Transactions.cs)

Tips for Debugging

  • The OpenPetra TDBTransaction Class (which is exclusively used wherever OpenPetra handles DB Transactions) has got three read-only Properties that can be of help especially when DB Transactions span several Methods. You can inspect those Properties when debugging ('Reused' can be enlightening when inspecting buggy DB Transaction behaviour!) and you could evaluate these Properties in very involved DB Transaction handling code. (Normally, there is no need to use these Properties, though.)
    • 'Reused': true if the DB Transaction has been re-used at least once by way of calling one of the 'TDataBase.GetNewOrExistingAutoTransaction' or 'TDataBase.GetNewOrExistingTransaction' Methods. Once this flag got set to 'true' it never gets set back to 'false'.
    • 'Valid': 'true' if the DB Transaction hasn't been Committed or Rolled Back, otherwise 'false'.
    • 'IsolationLevel': System.Data.IsolationLevel of the DB Transaction.