33

I've apparently worked myself into a bad coding habit. Here is an example of the code I've been writing:

using(StreamReader sr = new StreamReader(File.Open("somefile.txt", FileMode.Open)))
{
    //read file
}
File.Move("somefile.txt", "somefile.bak"); //can't move, get exception that I the file is open

I thought that because the using clause explicitly called Close() and Dispose() on the StreamReader that the FileStream would be closed as well.

The only way I could fix the problem I was having was by changing the above block to this:

using(FileStream fs = File.Open("somefile.txt", FileMode.Open))
{
  using(StreamReader sr = new StreamReader(fs))
  {
    //read file
  }
}

File.Move("somefile.txt", "somefile.bak"); // can move file with no errors

Should closing the StreamReader by disposing in the first block also close the underlying FileStream? Or, was I mistaken?

Edit

I decided to post the actual offending block of code, to see if we can get to the bottom of this. I am just curious now.

I thought I had a problem in the using clause, so I expanded everything out, and it still can't copy, every time. I create the file in this method call, so I don't think anything else has a handle open on the file. I've also verified that the strings returned from the Path.Combine calls are correct.

private static void GenerateFiles(List<Credit> credits)
{
    Account i;
    string creditFile = Path.Combine(Settings.CreditLocalPath, DateTime.Now.ToString("MMddyy-hhmmss") + ".credits");

    StreamWriter creditsFile = new StreamWriter(File.Open(creditFile, FileMode.Create));

    creditsFile.WriteLine("code\inc");

    foreach (Credit c in credits)
    {
        if (DataAccessLayer.AccountExists(i))
        {
            string tpsAuth = DataAccessLayer.GetAuthCode(i.Pin);
            creditsFile.WriteLine(String.Format("{0}{1}\t{2:0.00}", i.AuthCode, i.Pin, c.CreditAmount));
        }
        else
        {
            c.Error = true;
            c.ErrorMessage = "NO ACCOUNT";
        }

        DataAccessLayer.AddCredit(c);

    }

    creditsFile.Close();
    creditsFile.Dispose();

    string dest =  Path.Combine(Settings.CreditArchivePath, Path.GetFileName(creditFile));
    File.Move(creditFile,dest);
    //File.Delete(errorFile);
}
David Ferenczy Rogožan
  • 18,863
  • 8
  • 68
  • 65
scottm
  • 26,493
  • 22
  • 102
  • 155

5 Answers5

41

Yes, StreamReader.Dispose closes the underlying stream (for all public ways of creating one). However, there's a nicer alternative:

using (TextReader reader = File.OpenText("file.txt"))
{
}

This has the added benefit that it opens the underlying stream with a hint to Windows that you'll be accessing it sequentially.

Here's a test app which shows the first version working for me. I'm not trying to say that's proof of anything in particular - but I'd love to know how well it works for you.

using System;
using System.IO;

class Program
{
    public static void Main(string[] args)
    {
        for (int i=0; i < 1000; i++)
        {
            using(StreamReader sr = new StreamReader
                  (File.Open("somefile.txt", FileMode.Open)))
            {
                Console.WriteLine(sr.ReadLine());
            }
            File.Move("somefile.txt", "somefile.bak");
            File.Move("somefile.bak", "somefile.txt");
        }
    }
}

If that works, it suggests that it's something to do with what you do while reading...

And now here's a shortened version of your edited question code - which again works fine for me, even on a network share. Note that I've changed FileMode.Create to FileMode.CreateNew - as otherwise there could still have been an app with a handle on the old file, potentially. Does this work for you?

using System;
using System.IO;

public class Test
{    
    static void Main()
    {
        StreamWriter creditsFile = new StreamWriter(File.Open("test.txt", 
                                          FileMode.CreateNew));

        creditsFile.WriteLine("code\\inc");

        creditsFile.Close();
        creditsFile.Dispose();

        File.Move("test.txt", "test2.txt");
    }
}
Jon Skeet
  • 1,261,211
  • 792
  • 8,724
  • 8,929
  • Any idea why I can't move the file in the first block? – scottm Apr 01 '09 at 21:09
  • No - that should be okay. Odd. – Jon Skeet Apr 01 '09 at 21:10
  • Does it *always* fail with the first version and *always* work with the second? – Jon Skeet Apr 01 '09 at 21:12
  • Just tried the first version and it worked for me 20 times in a loop. – Jon Skeet Apr 01 '09 at 21:14
  • I've coded it the first way a hundred times, and never had a problem before. Now I'm opening the file on a windows share and this was the only way I could resolve the problem. It failed every time with the first example, and has worked every time with the second. So I thought I was doing it wrong – scottm Apr 01 '09 at 21:17
  • I can confirm Jon's observation, all three ways work. There must be something else affecting this. – Samuel Apr 01 '09 at 21:17
  • I just tested on a Windows share and all three ways worked again. – Samuel Apr 01 '09 at 21:20
  • Just tried it against a network share (albeit the share was a Linux box) and it worked okay then. The only difference is that in one case the stream is closed first, then the StreamReader. But StreamReader.Dispose will definitely call Stream.Close. Very weird. – Jon Skeet Apr 01 '09 at 21:25
  • I edited the question and added the actual block of code where this is happening (all names have been changed to protect the innocent). Maybe someone can shed some light on the flaw in my logic. – scottm Apr 01 '09 at 21:25
  • Just tried your example on the actual share this is happening on and there were no problems. After the app closes I can manually move the file with no problems – scottm Apr 01 '09 at 21:45
  • If you could keep reworking the code until you've got a short but *complete* program which demonstrates the problem (no need for actual credit stuff, unless something in there might touch the file) that's bound to help. – Jon Skeet Apr 01 '09 at 22:06
  • I think I'm done with it. I just wanted to make sure I wasn't completely wrong in assuming the FileStream was closed. Since the second block fixed the problem and it's not that important, I'm not going to delve any deeper. – scottm Apr 01 '09 at 22:19
  • Hmm... that sounds like a recipe for trouble later though. Any time you can't explain why one way works and another doesn't, that usually means there's a problem somewhere else which will exhibit itself differently later on. Ah well... it really *should* work. Does File.OpenText work btw? – Jon Skeet Apr 01 '09 at 22:21
  • Haven't tried that, maybe I will make some tests tomorrow and see if some of the other suggestions here work. – scottm Apr 01 '09 at 22:23
  • +1. @scotty dispose calls the inner stream.Close, which in turns calls its Dispose. I checked with reflector after JS comments on my answer (I deleted it, since it was wrong/misleading :(). – eglasius Apr 01 '09 at 22:26
  • What's your share hosted on, and what's your client OS? There can be some slight differences in cached writes between different setups of Windows (not to mention Samba, Netware, etc.). IIRC, there's a registry key to control it in Win2K+. – Mark Brackett Apr 01 '09 at 22:40
  • @Mark, it's a windows 2000 server machine. – scottm Apr 01 '09 at 22:42
12

Note - your using blocks do not need to be nested in their own blocks - they can be sequential, as in:

using(FileStream fs = File.Open("somefile.txt", FileMode.Open))
using(StreamReader sr = new StreamReader(fs))
{
    //read file
}

The order of disposal in this case is still the same as the nested blocks (ie, the StreamReader will still dispose before the FileStream in this case).

Not Sure
  • 5,415
  • 3
  • 20
  • 28
  • 7
    It looks better the other way to me. – scottm Apr 01 '09 at 22:17
  • 3
    Understand that this is no different than any other construct which has an initial clause, followed by a statement, where the statement can be a block. (A classic example is `if (list != null) foreach (object item in list) { ... }`, which is a `foreach` nested inside an `if`.) Specifically, this *is* "nesting" -- and a non-ideal indentation style, since it does not indicate the nesting. If you indent all but the first line, it is clearer what is going on: the second "using" is nested inside the first. – ToolmakerSteve Dec 19 '15 at 04:17
1

I would try to use FileInfo.Open() and FileInfo.MoveTo() instead of File.Open() and File.Move(). You could also try to use FileInfo.OpenText(). But these are just suggestions.

MartinStettner
  • 27,323
  • 13
  • 73
  • 104
  • I'll try the FileInfo methods, but I think underneath, they are just calls to File.Move – scottm Apr 01 '09 at 22:24
  • Yep, but perhaps they handle the stream (being created by FileInfo.Open()) internally. Also, documentation says they do some security checks only once, so it might be slightly faster ... – MartinStettner Apr 01 '09 at 22:40
0

Is there any possibility that something else has a lock to somefile.txt?

A simple check from a local (to the file) cmd line

net files

may well give you some clues if anything else has a lock.

Alternatively you can get something like FileMon to take even more details, and check that your app is releasing properly.

Zhaph - Ben Duguid
  • 25,726
  • 4
  • 75
  • 111
0

Since this doesn't seem to be a coding issue, I'm going to put my syadmin hat on and offer a few suggestions.

  1. Virus scanner on either the client or server that's scanning the file as it's created.
  2. Windows opportunistic locking has a habit of screwing things up on network shares. I recall it being mostly an issue with multiple read/write clients with flat file databases, but caching could certainly explain your problem.
  3. Windows file open cache. I'm not sure if this is still a problem in Win2K or not, but FileMon would tell you.

Edit: If you can catch it in the act from the server machine, then Sysinternal's Handle will tell you what has it open.

Mark Brackett
  • 81,638
  • 17
  • 102
  • 150
  • Very interesting. I've also had problems in the past in another application that I didn't code, but we found the culprit to be AVG. I'll have to look into that. – scottm Apr 03 '09 at 03:51