Do I have any SQL leaks?

Question

This is a Windows service that will run on about 600 machines. It is used to track the job server that a user is connected to (pushed through some kind of load balancer, not my area). I store this information in a SQL table and want to make sure that I don't have any SQL leaks.

this grabs information from the users computer (username/network name/AD Name) and requests a simple XML from the Load Balancer (which points to one of the job servers, all of which have a different value in the XML node retrieved) and then sends that information to a stored procedure which updates or inserts the information into the table, none of this happens if the application is not open.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Xml;
using System.Xml.XPath;
using System.Xml.Serialization;
using System.Data.Sql;
using System.Xml.Linq;
using System.Diagnostics;
using System.Data.SqlClient;
using System.Data.ProviderBase;
using System.Data.Common;
using System.Data.SqlTypes;
using System.Management;
using System.Net;
using System.Threading;

namespace AppServerTracker
{
    class Tracker
    {
        int intConnCount;
        string strServer;
        string strUserName;
        Process[] process = Process.GetProcessesByName("MShell");
        public static string strInput = "http://Website.for.load.Balancer.com";

        string strErrorMSG = "No Error"; //will show that there has been no error in the table.

        private static string SQLConnectionString = Properties.Resources.ConnStage;
        public SqlConnection SQLConnection = new SqlConnection(SQLConnectionString);

        public Tracker()
        {

        }

        public void Main()
        {
            if (process.Length > 0)
            {
                intConnCount = process.Length;

                ManagementScope ms = new ManagementScope("\\\\.\\root\\cimv2");
                ObjectQuery query = new ObjectQuery("SELECT * FROM Win32_ComputerSystem");
                ManagementObjectSearcher searcher = new ManagementObjectSearcher(ms, query);
                foreach (ManagementObject mo in searcher.Get())
                {
                    strUserName = mo["UserName"].ToString();
                }
                // Remove the domain part from the username
                string[] usernameParts = strUserName.Split('\\');
                // The username is contained in the last string portion.
                strUserName = usernameParts[usernameParts.Length - 1];

                string strComputerName = Environment.MachineName;

                if (isValid(strInput))
                {
                    try
                    {
                        XmlReader xmlReader = XmlReader.Create(strInput);
                        using (xmlReader)
                        {
                            while (xmlReader.Read())
                            {
                                if (xmlReader.NodeType == XmlNodeType.Text)
                                {
                                    strServer = xmlReader.Value.ToString();
                                    strServer = strServer.Replace("\r", "");
                                    strServer = strServer.Replace("\n", "");
                                    strServer = strServer.Replace(" ", "");
                                }
                            }
                            xmlReader.Close();
                        }
                    }
                    catch (Exception e)
                    {
                        strServer = "Error";
                        strErrorMSG = "Error with the xmlReader Exception as follows: " + e.ToString();
                    }
                    finally
                    {
                    }
                }
                else
                {
                    strServer = "XML-Missing";
                }

                try
                {
                    using (SQLConnection)
                    {
                        SQLConnection.Open();
                        using (SqlCommand TrackSproc = new SqlCommand("spServerTracking", SQLConnection))
                        {
                            TrackSproc.CommandType = System.Data.CommandType.StoredProcedure;
                            TrackSproc.Parameters.AddWithValue("@UserName", strUserName);
                            TrackSproc.Parameters.AddWithValue("@Server", strServer);
                            TrackSproc.Parameters.AddWithValue("@ConnCount", intConnCount);
                            TrackSproc.Parameters.AddWithValue("@MachineName", strComputerName);
                            TrackSproc.Parameters.AddWithValue("@Error", strErrorMSG);
                            TrackSproc.ExecuteNonQuery();
                        }
                    }
                }
                catch (Exception e)
                {
                    // E-mail Exception 
                }
                finally
                {
                    SQLConnection.Close();
                }
            }
        }

        public bool isValid(string url)
        {
            HttpWebRequest urlReq;
            HttpWebResponse urlRes;
            try
            {
                urlReq = (HttpWebRequest)WebRequest.Create(url);
                urlReq.Method = "HEAD";
                urlReq.Timeout = 100000;
                urlRes = (HttpWebResponse)urlReq.GetResponse();
                urlRes.Close();
                return true;
            }
            catch (Exception ex)
            {
                //Url not valid
                strErrorMSG = "Exception From isValid Method. Exception to follow: " + ex.ToString();
                return false;
            }

        }
    }
}

Here is the Entire code for this class.

• I am assuming that some of my Using statements are extraneous.

• assuming from comments already made that I should move the connection to the using statement and not make it accessible anywhere else because it isn't needed

• I removed comments that were extraneous (Mentioned in some comments listed on this post)

• I have an annoying naming scheme going on here I know, it was a bad habit I picked up from school. I will go through when I am through with this review and correct the issue, naming is the hard part.

Answer 1

Your code is asymmetrical....

You open your SQLConnection inside the using block, but you close it outside in the final block.... anyway, the Close is completely redundant:

If the SqlConnection goes out of scope, it won't be closed. Therefore, you must explicitly close the connection by calling Close or Dispose. Close and Dispose are functionally equivalent.

You are already disposing the connection inside the try block (as part of the using) so it is all moot.

The SQLConnection .... how is it created??? You have it magically appearing in the using preamble.... need ... more ... information.

Why do you have commented-out old code inside the block. If you are not using the sqlCommand variable any more (since it moved to the outer using block), then kill it, don't comment out the line.

Without knowing more about what the stored procedure does, it is hard to determine whether there are any other problems, but, your code could simply look like (although as the program exits the using block the SQLConnection will be disposed. This is an asymmetrical situation... the connection was open when the block started, and we are not returning it open):

try
{
    using (SQLConnection)
    {
        SQLConnection.Open();
        using (SqlCommand TrackSproc = new SqlCommand("spServerTracking", SQLConnection))
        {
            TrackSproc.CommandType = System.Data.CommandType.StoredProcedure;
            TrackSproc.Parameters.AddWithValue("@UserName", strUserName);
            TrackSproc.Parameters.AddWithValue("@Server", strServer);
            TrackSproc.Parameters.AddWithValue("@ConnCount", intConnCount);
            TrackSproc.Parameters.AddWithValue("@MachineName", strComputerName);
            TrackSproc.Parameters.AddWithValue("@Error", strErrorMSG);
            TrackSproc.ExecuteNonQuery();
        }
    }
}
catch (Exception e)
{
    // E-mail Exception 
}

Answer 2

It's not clear whether your SqlConnection is reused: it would be reused, if the client code (which uses Tracker) instantiates a Tracker instance and then calls Tracker.Main more than once.

IMO no object should be reused after it has been disposed. For example MSDN suggests a boolean inside the class to ensure that it's only disposed once. If this (legal) pattern is implemented in the SqlConnection class, it won't properly dispose objects that are re-acquired (if you reuse it) after it as been disposed.

It's possible that in practice SqlConnection can be reused safely after it has been disposed; however in theory that is undefined behaviour. MSDN for SqlConnection.Dispose says,

Call Dispose when you are finished using the Component. The Dispose method leaves the Component in an unusable state. After calling Dispose, you must release all references to the Component so the garbage collector can reclaim the memory that the Component was occupying. For more information, see Cleaning Up Unmanaged Resources and Implementing a Dispose Method.

It would be safer if your SQLConnection object were a local variable:

using (SqlConnection sqlConnection = new SqlConnection(SQLConnectionString)) { ... }

If you want to keep SQLConnection as an instance member of Tracker, it would be idiomatic for Tracker to implement IDisposable, and to call SQLConnection.Dispose from your Tracker.Dispose.