I have a task that I am trying to complete and would appreciate direction, to know which is the most efficient path to take for future requirements.
Use Case: A background process that determines if a specific database is broken. This task will be run every two minutes as we must know ASAP if there is a problem with the database.
My Solution: Opening a connection with said database, if the conenction is refused - an email is then sent notifying the appropriate individuals.
Here is the method I built to determine the DB state:
public bool DatabaseConnection()
{
bool statusUp = true;
using (var databaseConnection = new SqlConnection(ConfigData.ConnectionStrings.DatabaseConnectionString))
{
try
{
databaseConnection.Open()
}
catch (SqlException ex)
{
const string message = "Could not establish a connection with Database.";
Log.DatabaseStatusDown(message, ex);
statusUp = false;
}
finally { databaseConnection.Close(); }
}
return statusUp
}
I am aware that the Using statement leverages the IDisposable class and will auto-dispose the connection, but I wanted to be extra cautious and included the finally statement out of paranoia.
If anyone could please give me criticism, suggestions, recommendations or really anything that can help me improve this code I would greatly appreciate it. I am also open to different ideas on how I can determine if a DB is broken.
-
\$\begingroup\$ Would a better way not to keep the connection open and just hook in to the connection close event? You'd know instantly about any outages then. \$\endgroup\$Jack Wilsdon– Jack Wilsdon2015年05月07日 15:22:03 +00:00Commented May 7, 2015 at 15:22
-
\$\begingroup\$ Welcome to CodeReview, jayoharedee. This seems to be a really good question, I hope you get a really good answer. \$\endgroup\$Legato– Legato2015年05月07日 15:43:13 +00:00Commented May 7, 2015 at 15:43
-
\$\begingroup\$ Hi jackwilsdon and Legato, thank you for the messages! jackwilsdon - from my limited understanding of databases, connections should only be opened for a desired task and then closed immediately on completion. Would there be an advantage of keeping a connection always open? Maybe I am not quite sure on what exactly you are referring to... \$\endgroup\$jayoharedee– jayoharedee2015年05月07日 15:52:59 +00:00Commented May 7, 2015 at 15:52
-
\$\begingroup\$ I'd say you're right Jayoharedee, connections should opened, used and closed ideally. \$\endgroup\$Ben Whyall– Ben Whyall2015年05月07日 16:22:20 +00:00Commented May 7, 2015 at 16:22
2 Answers 2
Hmmm,
There are a number of problems with your check.
1: loss of network connectivity - DB may not be broken, but test will pass/fail, email not sent depending on what box your test is running on
2: Database Table is deadlocked or other DB problem - Simple connection test will pass
3: box on which test runs has crashed - test not run, no alert
4: DB down for maintenance - lots of emails sent
There are infrastructure monitoring tools you can buy/download to do this kind of thing and let you set up multiple tests and conditions etc. Best just to use an off the shelf product
As to your code, I would add:
bool statusUp = false; is better as it returns false if the test fails. set to true if it passes
If your code fails on the Connection constructor (say it cant read the config for example) the exception is not caught
-
\$\begingroup\$ Thanks for your answer. Yes number four is one that I am currently faced with. I was thinking to just pause the process during maintenance periods. \$\endgroup\$jayoharedee– jayoharedee2015年05月08日 16:21:09 +00:00Commented May 8, 2015 at 16:21
-
\$\begingroup\$ Have you used any infrastructure monitoring tools that you know could test for db problems? \$\endgroup\$jayoharedee– jayoharedee2015年05月08日 16:21:34 +00:00Commented May 8, 2015 at 16:21
-
\$\begingroup\$ If I put the
using
statement within the try/catch, would this solve the exception not caught on ur misconfig example? \$\endgroup\$jayoharedee– jayoharedee2015年05月12日 19:28:01 +00:00Commented May 12, 2015 at 19:28 -
\$\begingroup\$ I would go even higher, you must have a service or app which runs the method \$\endgroup\$Ewan– Ewan2015年05月14日 16:30:56 +00:00Commented May 14, 2015 at 16:30
Closing the database connection in the finally
will cause an exception if the Open ()
call throws an exception, so you better close the connection immediately after you opened it in the try
block.
To overcome this problem in a clean way you should let the using
block handle it to close the database connection.
Method's should be named using a verb or verb phrase. So better name it e.g IsDatabaseServerAlive ()
.
Otherwise it looks ok for me.
-
\$\begingroup\$ The implied
finally
of theusing
construct should close the connection anyhow, so I'd forgo the.Close()
altogether. \$\endgroup\$Jesse C. Slicer– Jesse C. Slicer2015年05月08日 00:55:15 +00:00Commented May 8, 2015 at 0:55 -
\$\begingroup\$ @JesseC.Slicer Yes, I know but because the OP stated he knows this but wanted to be sure I wanted to show the possible problems. Nevertheless updated the answer. \$\endgroup\$Heslacher– Heslacher2015年05月08日 04:17:59 +00:00Commented May 8, 2015 at 4:17