"Cannot access a closed file" error

[evilmrhenry]

The code

DataStreams.Csv.CsvWriter writer = new DataStreams.Csv.CsvWriter("test.csv");
writer.Write("1");
writer.EndRecord();
GC.Collect();

Will give the error "Cannot access a closed file", with the StackTrace:
at System.IO.FileStream.Write(Byte[] array, Int32 offset, Int32 count)
at System.IO.StreamWriter.Flush(Boolean flushStream, Boolean flushEncoder)
at System.IO.StreamWriter.Dispose(Boolean disposing)
at System.IO.TextWriter.Dispose()
at DataStreams.Csv.CsvWriter.Dispose(Boolean disposing)
at DataStreams.Csv.CsvWriter.Finalize()

I would expect the Dispose method to either Flush the stream, Close it, or null it as appropriate before handing it to the parent classes.

I'm using version 5.14; I'm not sure if there's a newer one.

[shriop]

I'm not sure what you're trying to get at. Your simple usage is what's causing the problem. Either call Close on the writer object or use a "using" block. An object somewhere below StreamWriter is being GC'd out from under the StreamWriter object. Notice that it's not a CsvWriter method that's throwing the exception. If this happens, it's perfectly fine for it to throw nasty because of possible buffered data having not been written out to the file yet.

Bruce Dunwiddie

[evilmrhenry]

The actual problem, as I see it, is that finalizers shouldn't throw exceptions:
http://msdn.microsoft.com/en-us/library/bb386039.aspx

And, yes, the object isn't being closed. Our real application is huge; I spent two days examining our usages of CSVWriter, and didn't find any problems. All CSVWriter instances should end up closed, even through exceptions, but there's at least one code path that ends up forcing an IIS pool recycle.

[evilmrhenry]

While I'm here, why isn't there an IsClosed() function or property? The CSVWriter class throws an exception if you try writing to a closed file, but there's no way of checking that first.

[shriop]

I don't think you're reading into what I'm saying. That link that you're pointing to says that Dispose shouldn't raise exceptions. This code is not raising an exception. The Dispose method of System.IO.StreamWriter is throwing an exception. If you want to give someone grief about Dispose methods throwing exceptions, talk to MS. And I think you're misinterpreting the rule. Dispose should not throw an exception unless it is considered a fatal error, http://msdn.microsoft.com/en-us/library/system.object.finalize(v=vs.110).aspx . As I've already said, your code logic is flawed, and I consider this a valid fatal exception. I understand based on your response that this is an oversimplified example, but I think your actual application does have the same logical error.

The issue is that you need to call Close before the variable goes out of scope. If you do not call Close, then garbage collection is left to dispose of objects in whatever order it sees fit. This causes both a logical and practical issue in that you still have data sitting in memory that has not been written out to the file. My biggest suggestion is that if you can, use "using" blocks around all uses of CsvWriter to insure that Close will be called. If you can't do this because of how the object is being passed around between scopes, then you need to insure that the last caller calls Close. You will find that StreamWriter itself works this way and that my suggestion follows many best practices around insuring proper calls to Close/Dispose on objects.

Your suggestion around the IsClosed property seems to match the usage of SqlDataReader more than the usage of StreamWriter, which I think is a more valid comparison. This IsClosed property does not exist on StreamWriter. You could maybe make more of an argument around having this property on CsvReader, but really the need for it on SqlDataReader arises from needing to know if there's more data to be fetched, and different options and frameworks built around that need.

Bruce Dunwiddie