1

i would like to write an Client in C# which checks if a User is logged in on different Clients. The Client should run 24/7 and refreshes a Database with some State Information for each Client.

My Problem is: The Command Line Tool takes more and more Memory, so ill think that there is a Problem that i allocate Memory which never gets released.

I think it is that i am creating a ManagementScope, but i cannot all the Dispose() Method for it.

Here is my Code:

static void Main(string[] args)
    {

        Ping pingSender = new Ping();
        PingOptions options = new PingOptions();

        string sqlconnectionstring = "Data Source=(local)\\SQLEXPRESS;Initial Catalog=clientstat;User ID=...;Password=....;Integrated Security=SSPI";
        SqlConnection clientread = new SqlConnection(sqlconnectionstring);
        clientread.Open();

        // Use the default Ttl value which is 128,
        // but change the fragmentation behavior.
        options.DontFragment = true;

        string username = "";

        // Create a buffer of 32 bytes of data to be transmitted.
        string data = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
        byte[] buffer = Encoding.ASCII.GetBytes(data);
        int timeout = 120;

        while (true)
        {

            SqlCommand clientcommand = new SqlCommand("SELECT * FROM Client WHERE StateID = @stateid", clientread);
            clientcommand.Parameters.Add(new SqlParameter("stateid", 1));

            SqlDataReader clientreader = clientcommand.ExecuteReader();
            while (clientreader.Read())
            {
                string ipadress = Convert.ToString(clientreader["IP"]);
                string clientid = Convert.ToString(clientreader["ID"]);

                if (ipadress != string.Empty && clientid != string.Empty)
                {
                    // First Try To Ping Computer

                    PingReply reply = pingSender.Send(ipadress, timeout, buffer, options);
                    if (reply.Status == IPStatus.Success)
                    {

                        try
                        {

                            ManagementScope managementScope = new ManagementScope((@"\\" + ipadress + @"\root\cimv2"));
                            managementScope.Options.Username = "....";
                            managementScope.Options.Password = "...";
                            managementScope.Options.EnablePrivileges = true;
                            // ObjectQuery to Check if User is logged on
                            ObjectQuery objectQuery = new ObjectQuery("SELECT * FROM Win32_ComputerSystem");
                            ManagementObjectSearcher managementObjectSearcher = new ManagementObjectSearcher(managementScope, objectQuery);
                            ManagementObjectCollection querycollection = managementObjectSearcher.Get();

                            foreach (ManagementObject mo in querycollection)
                            {
                                // Check Here UserName
                                username = Convert.ToString(mo["UserName"]);
                                if (username != "")
                                {
                                    Console.WriteLine(ipadress + " " + username);

                                }

                            }
                            querycollection.Dispose();
                            managementObjectSearcher.Dispose();
                        }
                        catch (Exception x)
                        {
                            Console.WriteLine(x.Message);
                        }
                    }
                }
                else
                {
                    Console.WriteLine(clientid + " has no IP-Adress in Database");
                }
            }
            clientcommand.Dispose();
            clientreader.Close();
            clientreader.Dispose();
        }

    }

}

Any Ideas or Suggestions what i can improve here? Or what exactly could be a Problem?

Thanks in advance

derdida
  • 14,784
  • 16
  • 90
  • 139
  • You need to memory profile your application. Google that for .net. There are various techniques for memory monitoring. e.g what types of objects are allocated in Gen 0, 1,2 after n minutes and after m minutes – Samuel Nov 11 '14 at 13:17
  • I am using the Express Edition, and thought there is no Memory Monitoring included? – derdida Nov 11 '14 at 13:18
  • You'll need Ultimate Version for the Visual Studio profiler anyway. But there are other free ways. WinDBG and a diff tool, rammap (or vmmap, I confuse which of them can profile), etc. You just need to sink your brain into it. – Samuel Nov 11 '14 at 13:20
  • 2
    clientcommand is undisposed – Alex K. Nov 11 '14 at 13:23
  • Thanks. Changed that, but still increasing Memory. It started with about 8 Mbytes, and after 5 Minutes up to 50 Mbytes. – derdida Nov 11 '14 at 13:26
  • In addition to what AlexK said, create the clientcommand object outside your while loop. This way you are not recreating an object the whole time that doesn't change at all – Bernd Linde Nov 11 '14 at 13:26
  • Thanks, i will try that. But why i cannot Dispose the "managementScope" and "ManagementObjectSearcher"? I have the same Problem when ill try it with a using Statement on initialising. – derdida Nov 11 '14 at 13:28
  • Only classes which implemented IDisposable can be disposed. Using statement and calling dispose is almost same thing. :) However for better readability is using statement better. – Honza Kovář Nov 11 '14 at 13:30
  • The ManagementObject might be the problem, have a look on this related question (and answer...) : http://stackoverflow.com/questions/15697684/is-it-necessary-to-dispose-every-managementobject – Larry Nov 11 '14 at 13:31
  • @Bernd: If ill put the clientCommand outside, i need to restart the Client if a State from the Client gets changed. But i dont want to restart the Client if anythings gets changed from the Database. – derdida Nov 11 '14 at 13:33
  • Also check if your CPU usage does go high on runtime, you're using a endless loop without, as far as I can see, any interruption. You might add a little delay, otherwise you don't only use much of your CPU time, but you might also kind of flood the client. Just a recommendation, correct me please if I am wrong – Kimmax Nov 11 '14 at 13:37
  • you never get read of the managementScope object this might be it. – Pedro.The.Kid Nov 11 '14 at 13:38
  • Keep the command `SqlDataReader clientreader = clientcommand.ExecuteReader();` inside the loop, that way it gets refreshed with each loop – Bernd Linde Nov 11 '14 at 13:38
  • You can use Red Gate Memory Profiler trail version to profile your application. Just a side note, polling is always CPU hungry, I am really not sure what is the objective of this, instead of this can you put some process on the client terminals to log the req. info whenever someone logs into it. – Thangadurai Nov 11 '14 at 14:16
  • I think the SqlConnection creation, open and dispose should be included in the loop. Reusing the same opened connection again and again leads to memory leaks, so I updated my answer. – Larry Nov 11 '14 at 14:31
  • Any news ? I am curious about it. – Larry Nov 12 '14 at 07:44
  • Ill tried out now for 1 day with using CG.Collect() in every block. Worked here fine, only about 7 Mbyte of RAM Usage after 16 Hours. Changed now my code to open and close the SQL Connection inside my Loop, run atm for about 20 Minutes and currently looking that this solved the problem too. (without using CG) - Thanks - i will set your answer as accepted here, but thank you all guys for your support! – derdida Nov 12 '14 at 08:14

5 Answers5

2

Idea 1:

You have to Dispose the ManagementObject to release the unmanaged COM resources.

Unfortunately, there is a bug in the Dispose implementation of it. Here are more details about it.

Credits should go to this answer that provides a workaround using GC.Collect(). Unfortunately, it costs.

That's why it is better to use a counter to perform the GC.Collect every n loops, with a n value you will manually tune until the performances are acceptable.

Anyway, I would try to invoke the ManagementObject Dispose() using reflection.

Idea 2:

In general, re-using a opened connection for several queries is not good since it prevents the connection pooling mechanism to work as optimal. Therefore, the sqlconnection may retain resources if used so.

Instead, please include the SqlConnection create/open and close/dispose in the loop, as related to this question.

Community
  • 1
  • 1
Larry
  • 17,605
  • 9
  • 77
  • 106
  • You mean GC.Collect release the unmanaged (COM) memory? GC calls if included are mostly for test frameworks not for production code. – Mrinal Kamboj Nov 11 '14 at 14:07
  • Also GC.Collect is never suggested to be used without the generation that you may want to be collected, like 0,1,2 and even more important is do not decide by yourself which generation you want to release, because you would have no idea regarding the allocation and promotion of objects – Mrinal Kamboj Nov 11 '14 at 14:13
  • Of course no, the GC does not release any unmanaged resources, this is the .Dispose role. Dispose also unreferences things so the GC works as optimal. I agree with you about the GC usage, and I would like to know if this "new Dispose()" can be called using Reflection, so we could avoid to use GC. – Larry Nov 11 '14 at 14:21
1

You should use using (and not invoke Dispose(), it's not needed). The "new" issue would be the nesting, which will look like this:

using (SqlConnection ...) 
{
    using (SqlCommand ...)
    {
        using (SqlDataReader ...) 
        {
           ...
        }
    }
}

Basically, if you are instancing something which implements IDisposable, put a using there and be assured that .NET will handle memory for you (at least, it will try to).

Alex
  • 23,004
  • 4
  • 39
  • 73
  • 1
    If his code doesn't throw an exception, and he disposes of all the objects that implement IDisposable, then switching to `using ...` won't change anything. – Lasse V. Karlsen Nov 11 '14 at 13:30
  • Thanks. Should i use "using" with "managementScope" too? Because if ill try that there is an Error. Or how do i "free" the memory from "managementScope" or "managementScopeSearcher" ? – derdida Nov 11 '14 at 13:30
  • wrap a using around everything that implements IDisposable (if you're not sure, try: the compiler complains where it's not available) – Alex Nov 11 '14 at 13:31
  • No there is not any exception. The Code works fine (there is only a Problem with the Memory usage) - i would like to run this Client 24/7 so there should be not any memory allocation problem. – derdida Nov 11 '14 at 13:31
  • @Alex: there's nothing wrong with using `Dispose`. The `using` is nothing more than syntactic sugar. – code4life Nov 11 '14 at 13:34
  • This is the way to go IMHO. The SqlConnection creation / disposal process has to be included in the loop. – Larry Nov 11 '14 at 14:37
0

Try to add a GC.Collect() call after each top level iteration (just to diagnose the issue). See if the memory behaves the same. If not then you don't have an issue, the GC might just be optimistic and delay collections.

Each iteration uses a non trivial amount of space due to the data reader buffers and what not so if those are just not collected you will observe memory just increasing.

It is just a false alarm though. If your system becomes memory constrained or the app triggers some kind of internal GC threshold collection will happen just fine.

Adrian Zanescu
  • 7,907
  • 6
  • 35
  • 53
  • -1: `GC.Collect` in a loop is asking for all sorts of problems. It most definitely introduces the danger of slowing down the application to a crawl. And think of what GC.Collect does to your threads, if you're doing asynchronous operations. You have to be sparing with using GC.Collect. Putting it into a loop is a no-no. – code4life Nov 11 '14 at 13:45
  • Ok - after using this everything looks fine. Starting with 8 Mbytes of Ram and increasing every 5 Seconds with 12 Kbytes. (That should be the Console Output) - So you mean that i could use my code without using GC.Collect() , because the GC will have an delay here? – derdida Nov 11 '14 at 13:46
  • @code4life - i said to add it just to figure out the problem. not keep it in production. jeez. i'll edit for more clarity – Adrian Zanescu Nov 11 '14 at 14:01
  • @derdida - yes. the GC is non deterministic. it decides itself when to collect. that can be hours later. in theory if you have unlimited available memory the GC can decide to not collect at all. ever – Adrian Zanescu Nov 11 '14 at 14:02
  • If not in production then GC.Collect never solved the issue. It never pointed to the real issue that existed and worst part is it made the application slow – Mrinal Kamboj Nov 11 '14 at 14:18
  • I'm going to revert my -1. But honestly, use a memory profiler, don't bake in a `GC.Collect` just to see if memory grows/shrinks. That approach can lead you down a very nasty garden path. And honestly, `GC.Collect` won't tell you where the leak is happening. – code4life Nov 11 '14 at 20:13
0

I'm not sure why you're creating a new SqlCommand on each loop iteration.

Just parameterize the SqlCommand, and in the loop iteration, and set the parameters, rather than creating a new SqlCommand.

Do that, and let me know how the memory looks. Remember one more thing - the GC won't kick in until it kicks in (i.e. non-determinism rules are in effect). Unless you really want to run a GC.Collect in your loop (that's sheer madness, IMHO). Another words, the constant creation/disposal of the objects is probably making the memory grow. Remember that a naive Dispose isn't going to make the memory magically shrink. Also keep in mind the memory management model of .NET and you should be all right.

code4life
  • 15,655
  • 7
  • 50
  • 82
0

The reason you are getting this exception is you are using while(true) statement and you are not using break; anywhere in a loop to break it explicitly. So the while loop is executing in infinite times and thus taking whole lot of memory. I think you should use Windows Service instead of while(true) to run it 24/7 and do it's operation without exception.

sandip patil
  • 191
  • 4