Sign in to follow this  

[C#] Does my class leak?

This topic is 3724 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I'm writing a utility class to read a simple text file format from both C# and C++ code. It looks like this:
public class CVSReader
{
    public CVSReader(string _filename)
    {
        try { mFile = new StreamReader(_filename); }
        catch (FileNotFoundException) {}
        mTokenQueue = new Queue();
    }

    public string NextToken()
    {
        if(mFile == null)
            return null;

        // Queue is empty? Fill it with the current line
        if (mTokenQueue.Count == 0)
        {
            string line = mFile.ReadLine();

            if(line == null)
                return null;

            string [] tokens = line.Split(',');
            
            foreach(string token in tokens)
                mTokenQueue.Enqueue(token.Trim());
        }

        return mTokenQueue.Dequeue() as string;
    }

    private StreamReader mFile;
    private Queue mTokenQueue;
}



Is this class ok WRT resource leaks? Will the garbage collector take care automatically of my StreamReader and Queue objects? I guess the answer is yes for the queue, but I have a doubt about the file stream (since it's considered an "unmanaged resource" if I understand correctly) Do I need to manually Dispose() it or something? How bad is it if I do/don't ? Thanks! JA [Edited by - janta on September 30, 2007 8:27:27 PM]

Share this post


Link to post
Share on other sites
Don't take my word as scripture, but I think the Grabage Collector takes care of it automatically. That said, if you're really worried about it, you could have a destructor or a Dispose() method (possible inherit from IDisposable?). First, just check in Intellisense (best thing Microsoft ever did for the world) that the Queue and StreamReader have a Dispose() method (pretty sure they do) and then in the destructor or Dispose() method just call their individual dispose methods.

Share this post


Link to post
Share on other sites
For the most part you do not have to worry about memory leaks in C#, you do have to worry about resource leaks.

The following may cause a memory leak;
creating a 3rd party object
opening a file handle
allocating some memory (not instantiating an object but directly allocating memory)
opening a database connection
opening a network port

In these cases you want to make sure the resource is released before you allow your object to be disposed.

theTroll

Share this post


Link to post
Share on other sites
There is actually a way to have some sort of memory leak in GC languages such as java and C#.


class SimpleStack<T>
{
// Details omitted, like growing the array when necessary
public void Push(T item) { _data[_idx++] = item; }
public T Pop() { return _data[--_idx]; }

private int _idx;
private T[] _data;
}

SimpleStack<BigObject> stack = new SimpleStack<BigObject>();
stack.Push( new BigObject() );
stack.Pop();


One could say this stack is "leaking memory", since it keeps references to objects which it does not hold; because of this the created BigObject will remain in memory as long as you don't overwrite the stack's internal reference with null or another BigObject reference. Better code would be

public T Pop()
{
T result = _data[_idx];
_data[_idx] = null;
--_idx;
return result;
}




However, this does not apply to the OP's CVSReader. I would add an IDisposable implementation to the class though, so that one can explicitly throw the CVSReader instance away. Theoretically, the GC will indeed close the stream and such, but there's absolutely no guarantee of when this will happen. This means you might be blocking the file for other uses while you're not even using it anymore.

Also, if you were writing to a stream, you'd definitely need a Dispose() method or something similar, as AFAIK, finalizers/destructors are not guaranteed to run on program shutdown* (I think you can ask .NET to do this, but this needs to be done explicitly). This means that if the GC didn't get to closing your file before your application ends, the buffered data will not have been written away.


public static void Main()
{
StreamWriter writer = new StreamWriter( @"e:\test.txt" );

writer.WriteLine( "Hello world" );
}


After running this little program, I found my test.txt file created, but empty. Adding a call to Dispose() (directly or using using, or calling Flush() or Close()) resolves the issue.

* If finalizers/destructors are not necessarily called during the lifetime of your application, you could wonder why this is not a problem when reading from streams/files. Simple: .NET won't close your file, but the underlying OS will. Writing is a problem because the OS doesn't know about the data buffered by .NET classes.

Share this post


Link to post
Share on other sites
Thanks Sam, what you suggest is pretty much what I did yesterday. I refactored my class so that it is cleaner and shorter. Also, I dropped it's ability to read the text file progressively.

Feel free to comment!

public class CSVReader
{
public CSVReader(string _filename)
{
try
{
using (StreamReader file = new StreamReader(_filename))
{
mTokenQueue = new Queue();

do
{
string line = file.ReadLine();
if (line == null)
break;

line = line.Trim();
if (line[0] == '#' || line != "")
continue;

char[] separators = { ',' };
string[] tokens = line.Split(separators);

foreach (string token in tokens)
mTokenQueue.Enqueue(token.Trim());
}
while (!file.EndOfStream);
}
}
catch(FileNotFoundException)
{
}
}

//---------------------------------------------

public string NextToken() { return NextToken(true); }
public string NextToken(bool _remove)
{
if (mTokenQueue == null || mTokenQueue.Count == 0)
return null;
else
return (_remove ? mTokenQueue.Dequeue() : mTokenQueue.Peek()) as string;
}

//---------------------------------------------

private Queue mTokenQueue = null;
}





Still not entirely satisfied though:
- I wish the try/catch block only enclosed the potentially exception throwing code and not the whole function
- StreamReader creationcould fail for another reason, like the file is already opened without being shared for reading.

Going to work on this tonight...

Share this post


Link to post
Share on other sites
Quote:
Original post by janta
Thanks Sam, what you suggest is pretty much what I did yesterday. I refactored my class so that it is cleaner and shorter. Also, I dropped it's ability to read the text file progressively.

Feel free to comment!
*** Source Snippet Removed ***

Still not entirely satisfied though:
- I wish the try/catch block only enclosed the potentially exception throwing code and not the whole function
- StreamReader creationcould fail for another reason, like the file is already opened without being shared for reading.

Going to work on this tonight...


Some suggestions:

  • I really really don't like you eating your exception like that (catch ( FileNotFoundException ) { }). C#'s not java, you don't have to make the compiler shut the **** up about those ************ checked exceptions (I really really hate those). Could you tell me why you are doing that in C#? I could still understand you'd want a non-existing file be equivalent with an empty file, but for that to happen you'd at least create your token queue. I see now that you take this being null into account in your other methods. Well... all right it works. But I believe it would be simpler to have a non-existing file result in an existing empty queue instead of null: if you have to add new methods in the future, you'll always have to deal with the possibility of the queue being null, which complicates things a bit. Not much of course, but I always try to choose for the design where it simply is impossible for me to forget about it.

  • if (line[0] == '#' || line != "") continue;

    continue means "skip the next part of the loop". So, only if the line is empty will it proceed to split the line and put the tokens in the queue. I'm thinking you need to write line == "" instead.

  • Again
    if (line[0] == '#' || line != "") continue;

    Keep in mind line might be equal to "". Accessing the first character (line[0]) will fail in that situation. I would suggest you change it to if ((line != "" && line[0] == "#") || ...) { }. If I was right in the point before (turning line != "" into line == "") you can simplify it all to
    if ( line == "" || line[0] == '#' ) continue;

    because it is guaranteed that the || operator short-circuits.

  • You're using a do while loop. This means your code will immediately try to read the first line of the file, split it, etc. and then only check if there's still something left in the file. But what happens if the file is empty to begin with? Shouldn't you be using a while loop? Minimal change and it works with empty files (it will result in an empty queue).

Share this post


Link to post
Share on other sites
Thanks for doing the debugging [smile]. In fact I posted that code in a hurry before going to my job this morning, without testing it so I think I'd eventually have caught those (points 2 and 3)

Point 4: if the file's empty, I thought the "if (line == null) break;" would handle that. I havent made my tests though.

Regarding point #1. Yes, I want a non existing file to be treated like there's no file (I could add some error logging later) You'd fint it cleaner that the Queue is always created? I though that the "mTokenQueue == null" condition in NextToken() would handle it easily.

Well, yet another version:


public class CSVReader
{
public CSVReader(string _filename)
{
mTokenQueue = new Queue();

try
{
StreamReader file = new StreamReader(_filename);
}
catch(FileNotFoundException)
{
}

while (!file.EndOfStream);
{
string line = file.ReadLine().Trim(); // assume line != null is ok then...?
if (line == "" || line[0] == '#')
continue;

char[] separators = { ',' };
string[] tokens = line.Split(separators);

foreach (string token in tokens)
mTokenQueue.Enqueue(token.Trim());
}
}

//---------------------------------------------

public string NextToken() { return NextToken(true); }
public string NextToken(bool _remove)
{
if (mTokenQueue.Count == 0)
return null;
else
return (_remove ? mTokenQueue.Dequeue() : mTokenQueue.Peek()) as string;
}

//---------------------------------------------

private Queue mTokenQueue = null;
}

Share this post


Link to post
Share on other sites
Quote:
Original post by janta
Thanks for doing the debugging [smile]. In fact I posted that code in a hurry before going to my job this morning, without testing it so I think I'd eventually have caught those (points 2 and 3)

Point 4: if the file's empty, I thought the "if (line == null) break;" would handle that. I havent made my tests though.

Regarding point #1. Yes, I want a non existing file to be treated like there's no file (I could add some error logging later) You'd fint it cleaner that the Queue is always created? I though that the "mTokenQueue == null" condition in NextToken() would handle it easily.

Well, yet another version:


...


About point 1, I was still updating my previous post :) (my posts are often only final about half an hour or so... I should really try to get it right the first time)

Point 4
if (line == null) break; would indeed work. From MSDN
Quote:

Return Value
The next line of characters from the input stream, or a null reference (Nothing in Visual Basic) if no more lines are available.


Point 1
It's mostly subjective. It can look unclean to create an object just for the sake of it, but that's something I don't care about. My belief is that code should be as simple as possible. So, as mentioned in my (edited) post above, you're potentially complicating the rest of your code by introducing an extra possible case to keep mind (null queue, empty queue, filled queue).
For example, it is common practice to define a "Null object" for certain interfaces, which does nothing. Instead of using null, you pass one such dummy object which mindlessly swallows every request. This way, your code doesn't need any conditional branches to deal with a possible null reference in every other method.

This technique has even be promoted to a design pattern I see. Behold: the Null Design Pattern!

edit:edit:edit:edit: I now pronounce this post final.

Share this post


Link to post
Share on other sites

This topic is 3724 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this