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;
}
[C#] Does my class leak?
Author
I'm writing a utility class to read a simple text file format from both C# and C++ code. It looks like this:
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]
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.
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
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
There is actually a way to have some sort of memory leak in GC languages such as java and C#.
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
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.
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.
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.
Author
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!
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...
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...
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 toif ( 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).
Author
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:
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;}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.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
