5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ if you are going to downvote please explain why so that I can form better questions in the future \$\endgroup\$ – Malachi Mar 4 '14 at 22:42
  • \$\begingroup\$ I downvoted because I didn't think it was enough code to be considered "working code": when/how is the SQLConnection created? Does it even "work", given that you call a method on it (i.e. Close) after it has been Disposed? \$\endgroup\$ – ChrisW Mar 4 '14 at 22:50
  • \$\begingroup\$ it runs. it has been running on about 20 machines but they are having issues with one of the SQL Servers and I wanted to make sure that this service isn't leaving a connection open into the server. \$\endgroup\$ – Malachi Mar 5 '14 at 1:36
  • 1
    \$\begingroup\$ I don't have the Code here to post the rest of it. The connection object still exists, so it won't throw a huge fit if you try to close and already closed connection in a finally statement, so that is inefficient use of 3 lines in my code. post that as an answer. The code works as is though. I also agree (after seeing it posted here) that SQLConnection is not a good name for that connection Variable. \$\endgroup\$ – Malachi Mar 5 '14 at 2:20
  • 1
    \$\begingroup\$ Therefore I removed the downvote. Thanks. \$\endgroup\$ – ChrisW Mar 5 '14 at 16:56
6
\$\begingroup\$

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 
}
\$\endgroup\$
  • \$\begingroup\$ I want to make sure that no matter what that connection is closed. but I know that the using block automatically closes the Connection. I guess you could call it overkill, I don't want it to come back and bite me that there is a SQL leak. as for the commented out code, gone now. \$\endgroup\$ – Malachi Mar 4 '14 at 22:42
  • 1
    \$\begingroup\$ Looking at the code, it appears that you may have a static SqlConnection somewhere, I would avoid that practice altogether and just instantiate a new one in the using block any time you really need it. That way you aren't just passing things around blindly and you keep track of your objects. \$\endgroup\$ – Evan L Mar 5 '14 at 0:02
  • \$\begingroup\$ there are two things that this service does, 1. gather information 2. send the information to a Sproc, that is what this portion of the code looks like, I will post the rest of my code tomorrow because hearing what you said @EvanL made me think about where I want to create this connection. the reason is that this service runs about every 10 minutes if a certain application is open on the users machine. \$\endgroup\$ – Malachi Mar 5 '14 at 2:23
  • 2
    \$\begingroup\$ @Malachi using ensures that your SQLConnection will close even if an exception occures. \$\endgroup\$ – Max Mar 5 '14 at 8:30
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I think that I want to move the connection inside the using statement. I want it created and disposed right there. so the next time the class is called it creates and destroys a new connection. \$\endgroup\$ – Malachi Mar 5 '14 at 22:47

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.