An absolute beginner needs help with C#

[SpaghettiCarbonara]

I've just started learning C# from Barnecules' codegasm videos and I've completed his last video in which he shows how to make a hard drive activity indicator that shows up in the notification area but I've noticed a little problem with it. Just after it starts, the memory usage starts at around 13,000 K and gradually increases to around 23,000 - 25,000 K before jumping back down to around 14,000 K. This gradual increase continues, never returning to 13,000 K. One guy in the comments of the video said it eventually crashed. Is this a memory leak?
 
I've poked around a bit but I have no idea how to solve this. I'm using Visual Studio 2015.
 

 

while (true)
{

// Connect to the drive performance instance
ManagementObjectCollection driveDataClassCollection = driveDataClass.GetInstances();
foreach (ManagementObject obj in driveDataClassCollection)

{

if ( obj["Name"].ToString() == "_Total")
{

if(Convert.ToUInt64(obj["DiskBytesPersec"]) > 0)
{

// Show busy icon
hddNotifyIcon.Icon = busyIcon;

}

else
{

// Show idle icon
hddNotifyIcon.Icon = idleIcon;

}

}

 

}
// Sleep for 10th of a second
Thread.Sleep(100);

}

 

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Management;
using System.Management.Instrumentation;
using System.Collections.Specialized;
using System.Threading;

namespace HDAI
{

public partial class InvisibleForm : Form
{

#region Global Variables
NotifyIcon hddNotifyIcon;
Icon busyIcon;
Icon idleIcon;
Thread hddInfoWorkerThread;
#endregion

#region Main Form (entry point)
public InvisibleForm()
{

InitializeComponent();

//Load icons from files into objects
busyIcon = new Icon("HDAIBusy.ico");
idleIcon = new Icon("HDAIIdle.ico");

// Create notify icons and assign idle icon and show it
hddNotifyIcon = new NotifyIcon();
hddNotifyIcon.Icon = idleIcon;
hddNotifyIcon.Visible = true;

// Create all context menu items and add them to notification tray icon
MenuItem progNameMenuItem = new MenuItem("Hard Drive Activity Indicator v1.0 by Michael");
MenuItem quitMenuItem = new MenuItem("Quit");
ContextMenu contextMenu = new ContextMenu();
contextMenu.MenuItems.Add(progNameMenuItem);
contextMenu.MenuItems.Add(quitMenuItem);
hddNotifyIcon.ContextMenu = contextMenu;

// Wire up quit button to close application
quitMenuItem.Click += quitMenuItem_Click;

//
// Hide the form because we don't need it, this is a notification tray application
//

this.WindowState = FormWindowState.Minimized;
this.ShowInTaskbar = false;

// Start worker thread that pulls HDD activity
hddInfoWorkerThread = new Thread(new ThreadStart(HDDActivityThread));
hddInfoWorkerThread.Start();

}

#endregion

#region Context Menu Event Handlers
///

/// Close the application on click of 'quit' button on context menu
///
///
///
private void quitMenuItem_Click(object sender, EventArgs e)
{

 

hddInfoWorkerThread.Abort();
hddNotifyIcon.Dispose();
this.Close();

}
#endregion

#region Hard drive activity thread

///

/// This is the thread that pulls the HDD for activity and updates the notification icon
///
public void HDDActivityThread()
{

 

ManagementClass driveDataClass = new ManagementClass("Win32_PerfFormattedData_PerfDisk_PhysicalDisk");
 

try
{

// Main function loop
while (true)
{

 

// Connect to the drive performance instance
ManagementObjectCollection driveDataClassCollection = driveDataClass.GetInstances();
foreach (ManagementObject obj in driveDataClassCollection)

{

if ( obj["Name"].ToString() == "_Total")
{

if(Convert.ToUInt64(obj["DiskBytesPersec"]) > 0)
{

// Show busy icon
hddNotifyIcon.Icon = busyIcon;

}

else
{

// Show idle icon
hddNotifyIcon.Icon = idleIcon;

}

}

}

// Sleep for 10th of a second
Thread.Sleep(100);

}

} catch( ThreadAbortException tbe )

{

driveDataClass.Dispose();
// Thread was aborted

}

}

#endregion

}

 
}

[Nuluvius]

On breif look at that code and I'm not in the least bit surprised that there's a memory leak... that code is dire. I'm working right now so I don't really have the time to get into it.

[SpaghettiCarbonara]

On breif look at that code and I'm not in the least bit surprised that there's a memory leak... that code is dire. I'm working right now so I don't really have the time to get into it.

Ok no problem. Thanks dude, I have no idea with this stuff.

 

I've just completed the first part of the web development course on Codecademy and holy shit it's fun

[madknight3]

You can try using statements which will automatically dispose of the objects when done. I'm not positive if this will solve the issue as I haven't tested it myself, but give it a try.

using (ManagementClass driveDataClass = new ManagementClass("Win32_PerfFormattedData_PerfDisk_PhysicalDisk")){    // Rest of method}

using (ManagementObjectCollection driveDataClassCollection = driveDataClass.GetInstances()){    // rest of while loop}

You can also remove the try/catch as it's no longer needed.

[SpaghettiCarbonara]

You can try using statements which will automatically dispose of the objects when done. I'm not positive if this will solve the issue as I haven't tested it myself, but give it a try.

using (ManagementClass driveDataClass = new ManagementClass("Win32_PerfFormattedData_PerfDisk_PhysicalDisk")){    // Rest of method}

using (ManagementObjectCollection driveDataClassCollection = driveDataClass.GetInstances()){    // rest of while loop}

You can also remove the try/catch as it's no longer needed.

 

Alright, I'll make a copy of the program and try this. Thanks! Will report back.

 

Edit: How did you format the code there? I couldn't figure out how to do it in my OP Nevermind, I figured it out.

 

Edit 2: Ok so I've tried a few different things.

 

If I have both of those statements outside the loop, it actually makes the leak worse. Like this:

using (ManagementClass driveDataClass = new ManagementClass("Win32_PerfFormattedData_PerfDisk_PhysicalDisk")){     using (ManagementObjectCollection driveDataClassCollection = driveDataClass.GetInstances())    {        // Main function loop        while (true)        {

If I put the top one outside the loop and the bottom inside the loop, it slows the leak a lot, but it's still there. Like this:

using (ManagementClass driveDataClass = new ManagementClass("Win32_PerfFormattedData_PerfDisk_PhysicalDisk")){        // Main function loop        while (true)        {                 using (ManagementObjectCollection driveDataClassCollection = driveDataClass.GetInstances())                 {

[AndySw]

Does the memory now keep increasing forever until it goes pop or does it return back to a lower amount on a regular basis and then climb again?

 

If it keeps going up over time, even if it goes down a bit first, you've got a memory leak.

 

If it goes up and then back down to the same level repeatedly (no increase on the base level over time), then it's probably fine - it's the garbage collector doing it's thing to automatically manage memory for you.

 

https://msdn.microsoft.com/en-us/library/0xy59wtx(v=vs.110).aspx

[SpaghettiCarbonara]

Does the memory now keep increasing forever until it goes pop or does it return back to a lower amount on a regular basis and then climb again?

 

If it keeps going up over time, even if it goes down a bit first, you've got a memory leak.

 

If it goes up and then back down to the same level repeatedly (no increase on the base level over time), then it's probably fine - it's the garbage collector doing it's thing to automatically manage memory for you.

 

https://msdn.microsoft.com/en-us/library/0xy59wtx(v=vs.110).aspx

 

It gradually increases over time while also jumping back down close to the original amount.

 

I.e. It starts at 13,000 K, increases up to 23,000-25,000 K and jumps back down to 14,000 K, then increases back up to 23,000-25,000 K and jumps back down to 15,000 K and so on.

[Nuluvius]

It gradually increases over time while also jumping back down close to the original amount.

 

I.e. It starts at 13,000 K, increases up to 23,000-25,000 K and jumps back down to 14,000 K, then increases back up to 23,000-25,000 K and jumps back down to 15,000 K and so on.

 

I still don't have any free time unfortunately. But yes that behaviour is indicative of a memory leak. The nondeterministic nature of the garbage collector in .Net is why you see the large periods of increase then sudden decrease but it shouldn't continue to rise even by such a small amount. That management stuff is crap in any event.

[AndySw]

Leave it running for a few hours, see what happens. .NET does have some memory profiling tools and you can try using Performance Monitor to figure out what is going on. I haven't looked at the code, but just make sure you're disposing of anything that needs clearing up - the 'using' block is your friend here.

[SpaghettiCarbonara]

I still don't have any free time unfortunately. But yes that behaviour is indicative of a memory leak. The nondeterministic nature of the garbage collector in .Net is why you see the large periods of increase then sudden decrease but it shouldn't continue to rise even by such a small amount. That management stuff is crap in any event.

 

I understand half of those words. What am I saying?

 

But nah take your time dude, it's not urgent.

 

Leave it running for a few hours, see what happens. .NET does have some memory profiling tools and you can try using Performance Monitor to figure out what is going on. I haven't looked at the code, but just make sure you're disposing of anything that needs clearing up - the 'using' block is your friend here.

 

Will do. I'll update later.

 

Edit: I'm getting some weird things going on. The performance is different every time I run in debug mode with the same code

[AndySw]

Try a 'Release' build - it's slightly optimised AFAIK.

[KB_TheDireWolf]

Yah, am going to stay away from coding. Just going stick to building PC and getting them running with stuff that was pre-coded by people who know what their doing lol.

[madknight3]

Yah, am going to stay away from coding. Just going stick to building PC and getting them running with stuff that was pre-coded by people who know what their doing lol.

 

There are plenty of people out there who don't know what they are doing and still releasing products. You could be using one of those products, you never know

[Nuluvius]

Try a 'Release' build - it's slightly optimised AFAIK.

 

:blink:

 

No this is not the right thing to do at all. At best all you will be doing is hiding the issue. You should always investigate and fully understand the problem, never opt to hide it because it will come back to bite you in the arse later. Do not get into this kind of habit. 

[Nuluvius]

I've cleaned up your code and distilled the problem. I haven't had time to profile it but it might be worth doing some research yourself.

using System;using System.Management;using System.Threading;using System.Threading.Tasks;namespace ConsoleApplication1{    class Program    {        static void Main(string[] args)        {            var cts = new CancellationTokenSource();            Task.Factory.StartNew(() =>            {                using (var driveDataClass = new ManagementClass("Win32_PerfFormattedData_PerfDisk_PhysicalDisk"))                {                    while (true)                    {                        using (var instances = driveDataClass.GetInstances())                        {                            foreach (var instance in instances)                            {                                if (instance["Name"].ToString() == "_Total")                                {                                    Console.WriteLine(Convert.ToUInt64(instance["DiskBytesPersec"]) > 0 ? "Busy" : "Idle");                                }                                instance.Dispose();                            }                        }                        if (cts.Token.IsCancellationRequested)                        {                            return;                        }                        // GC.Collect();                        Thread.Sleep(100);                    }                }            }, cts.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default);            Console.ReadKey();            cts.Cancel();            Console.ReadKey();        }    }}

Note that I've removed the use of Thread over Task (Task-based Asynchronous Pattern TAP) because that is what you should be using instead. Also NEVER abort a thread to cancel it... use the CancellationToken instead.

[Nuluvius]

Here is some profiling of my code for you:

 

 

Results for comparing the two most distant snapshots:

 

[SpaghettiCarbonara]

I've cleaned up your code and distilled the problem. I haven't had time to profile it but it might be worth doing some research yourself.

~snip~                            foreach (var instance in instances)                            {                                if (instance["Name"].ToString() == "_Total")                                {                                    Console.WriteLine(Convert.ToUInt64(instance["DiskBytesPersec"]) > 0 ? "Busy" : "Idle");                                }                                instance.Dispose();                            }~snip~

Note that I've removed the use of Thread over Task (Task-based Asynchronous Pattern TAP) because that is what you should be using instead. Also NEVER abort a thread to cancel it... use the CancellationToken instead.

 

Can I use the original function I had in place of the Console.Writeline? The program is supposed to show a little icon in the notification area that changes colour when the hard drive is in use.

 

I'll give those suggestions a go when I have time

 

Here is some profiling of my code for you:

 

~snip~

 

Results for comparing the two most distant snapshots:

 

~snip~

 

That seems like a lot of memory used for what the program does

 

 

Anyway sorry I haven't been back in a while, I had some stuff going on.

 

I've tried running the program for a good while (30mins to an hour or so) and I noticed it does its little memory hissy fits but it actually stops at around 23,000-25,000 K and stays there. It doesn't seem to go above that. (Edit: I just had a look at the link you gave me for my own research and it looks like this memory problem is just a .NET thing, as a guy was saying in that discussion. Would this be correct?)

 

Also I have no idea how to make a standalone installer for it. I tried looking around on the web and found nothing. I only found people saying to copy the .exe from the Release or Debug folder or whatever, but it crashes if it doesn't have the .ico files with it. I tried using the Publish function of VS and ended up with some weird CD installer but no standalone installer, like what you'd end up with if you downloaded it from the internet.

 

I'm so lost

[Nuluvius]

I'll give those suggestions a go when I have time

That seems like a lot of memory used for what the program does

Anyway sorry I haven't been back in a while, I had some stuff going on.

I've tried running the program for a good while (30mins to an hour or so) and I noticed it does its little memory hissy fits but it actually stops at around 23,000-25,000 K and stays there. It doesn't seem to go above that. (Edit: I just had a look at the link you gave me for my own research and it looks like this memory problem is just a .NET thing, as a guy was saying in that discussion. Would this be correct?)

Yeah I think it's actually fine. As I said, that WMI management stuff is absolute crap.

 

Also I have no idea how to make a standalone installer for it. I tried looking around on the web and found nothing. I only found people saying to copy the .exe from the Release or Debug folder or whatever, but it crashes if it doesn't have the .ico files with it. I tried using the Publish function of VS and ended up with some weird CD installer but no standalone installer, like what you'd end up with if you downloaded it from the internet.

I'm so lost

Regarding the installer it's very straightforward, have a look at WIX.

 

Can I use the original function I had in place of the Console.Writeline? The program is supposed to show a little icon in the notification area that changes colour when the hard drive is in use.

The idea was for you to encourporate the code.

You should make an async method that takes in a cancellation token and returns you a Task to which you await. You can then surround that await with a try catch.

[LukeTim]

<Nevermind>

[SpaghettiCarbonara]

Yeah I think it's actually fine. As I said, that WMI management stuff is absolute crap.

 

Regarding the installer it's very straightforward, have a look at WIX.

 

The idea was for you to encourporate the code.

You should make an async method that takes in a cancellation token and returns you a Task to which you await. You can then surround that await with a try catch.

 

Ok thanks for the help.

 

Sorry for the late reply, had other things going on.

[babadoctor]

The single biggest problem in communication is the illusion that it has taken place.

• QUOTE
• MULTIQUOTE
• REPORT

 

 

WHAT?

The single ... if it never took place then there is no communication!!!

so its not a flaw

;_;