4
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\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\$ May 7 '15 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
    May 7 '15 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\$ May 7 '15 at 15:52
  • \$\begingroup\$ I'd say you're right Jayoharedee, connections should opened, used and closed ideally. \$\endgroup\$
    – Ben Whyall
    May 7 '15 at 16:22
2
\$\begingroup\$

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

\$\endgroup\$
4
  • \$\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\$ May 8 '15 at 16:21
  • \$\begingroup\$ Have you used any infrastructure monitoring tools that you know could test for db problems? \$\endgroup\$ May 8 '15 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\$ May 12 '15 at 19:28
  • \$\begingroup\$ I would go even higher, you must have a service or app which runs the method \$\endgroup\$
    – Ewan
    May 14 '15 at 16:30
3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ The implied finally of the using construct should close the connection anyhow, so I'd forgo the .Close() altogether. \$\endgroup\$ May 8 '15 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
    May 8 '15 at 4:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.