DB Access Exception Handling Policy: Difference between revisions

From OpenPetra Wiki
Jump to navigation Jump to search
Line 1: Line 1:
==Overview==
==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:'''
'''The correct handling of Exceptions that occur because of database access is very important for the following reasons:'''
# DB Exception Handling '''must ensure''' that
# 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'' (in case of write access to the DB) ''or committed'' (in case of read access to the DB). That way a DB transaction can never be left running ('dangling') where it shouldn't.
## 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.
# 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.
# 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.




=== The Danger of 'Dangling DB Transactions' ===
=== The Danger of 'Dangling DB Transactions' ===
DB Transactions that are left running where they shouldn't (sometimes referred to as 'dangling DB Transactions') will 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...  
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 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 ...
* might be written in such a way that it can re-use an existing Transaction, but ...
** that 'piggy-backing of an existing 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.  
** that 'piggy-backing of an existing 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>).  
** 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, which 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 (esp. once the application is used by users)!
** 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, which 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 (esp. once the application is used by users)!




===DB Exception Handling===
===DB Exception Handling===
* '''Scenario 1''': just reading data
==== '''Scenario 1''': Only reading data====
** '''Way 1''': Commit before first Catch Clause + Commit in Catch Clause(s), no Finally Clause
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:
*** ''Advantage'':
 
**** No Finally Clause needed, therefore less deep nesting of code.  
'''Policy: DB Transaction Rollback only in Finally Clause which encloses try Clause or try-catch Clause'''.
*** ''Disadvantage'':
 
**** Commit can be accidentally 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!!!
''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 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!!!
Example: <code>\trunk\csharp\ICT\Petra\Server\lib\MCommon\Cacheable-generated.cs, Method 'GetCacheableTable')</code>
*** ''Disadvantage'': Finally Clause is needed, therefore deeper nesting of code.
 
==== '''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 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 cause 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 programmer forgot to set the Variable to <code>false</code> in an 'error condition'. The programmer is 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.
* 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>


'''Recommendation''' (ChristianK): We should discourage 'Way 1' and ensure that 'Way 2' is implemented everywhere.


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.


* '''Scenario 2''': Writing Data / mixed reading/writing of data
However, quite a lot of DB Transaction handling code in OpenPetra does not meet that policy at present (April 2014). 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 meet this policy in case it doesn't already meet this policy''. That way we the policy will be implemented more and more across the server-side code base.  
** '''Way 1''': Commit before first Catch Clause + Rollback in Catch Clause(s), no Finally Clause
*** ''Advantages'':
**** No Finally Clause needed, therefore 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 accidentally 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, therefore deeper nesting of code.
**** bool Variable needed.


'''Recommendation''' (ChristianK): We should discourage 'Way 1' and ensure that 'Way 2' is implemented everywhere.
For any new server-side code that contains DB Transaction handling code the policy is to be implemented right from the start, of course!





Revision as of 10:33, 29 April 2014

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.
  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.


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 'piggy-backing of an existing 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, which 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 (esp. once the application is used by users)!


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: \trunk\csharp\ICT\Petra\Server\lib\MCommon\Cacheable-generated.cs, Method 'GetCacheableTable')

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 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 cause 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 programmer forgot to set the Variable to false in an 'error condition'. The programmer is 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.
  • 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.

However, quite a lot of DB Transaction handling code in OpenPetra does not meet that policy at present (April 2014). 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 meet this policy in case it doesn't already meet this policy. That way we the policy 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 policy is to be implemented right from the start, of course!


DB Transaction Handling When DB Transactions Span Several Methods

TODO