0

I have an application where I need to write binary to a file constantly. The bits of data are small, about 1K each. The computers this is running on aren't great and are running XP. I've run into the problem that when I turn on the logging the computers just get totally hosed and I watch the Task Manager and just see the memory usage going up and up until it crashes.

A coworker suggested that I just keep the packets in memory until a certain amount of time has passed and then write it all at once instead of writing each one separately - tried that, same issue.

This is the code (loggingBuffer is the List<byte[]> I'm storing the packets in while the interval passes):

if ((DateTime.Now - lastStoreTime).TotalSeconds > 10)
{
    string fileName = @"C:\Storage\file";
    FileMode fm = File.Exists(fileName) ? FileMode.Append : FileMode.Create;
    using (BinaryWriter w = new BinaryWriter(File.Open(fileName, fm), Encoding.ASCII))
    {
        foreach (byte[] packetData in loggingBuffer)
        {
            w.Write(packetData);
        }
    }
    loggingBuffer.Clear();
    lastStoreTime= DateTime.Now;
}

Is there anything different I should be doing to accomplish this?

MethodMan
  • 18,625
  • 6
  • 34
  • 52
eddie_cat
  • 2,527
  • 4
  • 25
  • 43
  • 2
    Use an outer 'using' around the File.Open and explicitly close the opened file, e.g. using(FileStream fs = File.Open(etc.etc.)) { using (BinaryWriter w = new BinaryWriter(fs)) { – user469104 Jan 13 '16 at 20:13
  • `w.Write(packetData);` after this line try putting `w.Flush();` also look up the `File.WriteAllBytes(string path, byte[] bytes)` method – MethodMan Jan 13 '16 at 20:27

1 Answers1

1

Seems to me that, while you're writing each 10 seconds, you could close the file in between. And cleanup all related file-writing things. Perhaps that would solved your problem. Secondly, I'd suggest creating the BinaryWriter outside the function where you actually write the data. It'll keep things clearer. In your current code you're checking each time wether to append data or to create a new file and the write to it. If you'll do this outside the function and call it just once perhaps this will save memory too. All untested by me, that is :)

  • @HenkHolterman that's not true, at all. If you have an object that has a method that you're calling in a loop, it is pointless to put that object in the loop itself. You are creating thousands of handles for no reason. – Dispersia Jan 13 '16 at 20:29
  • I agree in keeping scopes as small as possible @HenkHolterman. Looking again at the code in the question, the BinaryWriter is never cleaned up, A `w.dispose` or `w = null` seems usefull to me. Isn't that the cause of the memory leak? –  Jan 13 '16 at 20:54
  • @HenkHolterman ,Raymond that's horrible design practice. If those handles don't need instantiated, don't instantiate them. That makes no sense to create them for no reason just to destroy them. Keep it as small as possible as long as it's **logical** why you're destroying it. You're literally filling RAM with objects that do nothing and expecting GC just to clean them, you don't see the issue in that? – Dispersia Jan 13 '16 at 21:06