# Implementing a cancellable “infinite” loop

I'm making a Windows service that watches a directory for new files to process them.

I hit a race condition between the file copying going on and me trying to process it. Going by a SO answer I wrote the following wait-and-retry method to make sure I can work with the file:

public static void WaitForFile(string file, CancellationToken token, int retryIntervalMillis = 200)
{
while (!token.IsCancellationRequested)
{
try
{
using (new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.None))
return;
}
catch
{
}
}

throw new OperationCanceledException(token);
}


My question is: would this be a correct way to make this waiting cancellable? The token is cancelled to shut down the service, the idea is to allow for a clean (-ish) shutdown even in a case a bug means the file in question is inadvertently kept open indefinitely.

(Or if the reason the file can't be opened is some other, non-intermittent condition I haven't guarded against yet.)

For instance, is it okay to use the token this way in my method, and also expect the same token to cancel a BlockingCollection's consuming enumerator?

My intent is to base the service around the expectation it can be hard-crashed at any time, so I'm fine with an exception taking out most of the call stack leading to it.

## Edit:

These are changes that I have since made to the code

const int ErrorSharingViolation = -2147024864;

public static FileStream WaitForFile(string file,
CancellationToken token,
FileMode mode = FileMode.Open,
Action<Stream> action = null,
int retryIntervalMillis = 200)
{
while (!token.IsCancellationRequested)
{
try
{
var stream = File.Open(file, mode, access, FileShare.None);
if (action != null)
{
using (stream)
{
action(stream);
}
}
return stream;
}
catch (IOException exception)
{
if (exception.HResult != ErrorSharingViolation)
{
throw;
}
else
{
}
}
}
throw new OperationCanceledException(token);
}

• It seems to me that you're only closing a newly created connection to the file not an existing one. You might have to pass the stream instead so that it can be closed. – tinstaafl Jan 8 '14 at 6:08
• @tinstaafl That's intentional. I want to wait until it's possible to create new connections to the file in the first place. The existing connection is made from Windows Explorer as it copies the file into the drop folder, not from my service. – millimoose Jan 8 '14 at 6:12
• What happens in the case of an exception, does the file become unusable until the program is closed? Also, in my opinion this usage of try/catch falls under "expected behaviour", is there not a way you can test the file without opening a stream? Wouldn't it be more common for an exception to occur than for it to not? – Sam Jan 8 '14 at 10:51
• – Bobby Jan 8 '14 at 15:01
• @Sam That's why the using is there – the file is opened, then immediately closed again so the caller can open it. I've since changed it to actually invoke a callback instead to avoid the (in my use case unlikely) race condition where another process may open the file between WaitForFile() closing and the caller opening it. – millimoose Jan 8 '14 at 18:16

Your catch statement is catching any error (EVERY ERROR), which is not a good thing. I assume there are several errors that you are anticipating, but you shouldn't catch them all. You will run into bugs and won't know what is going on because the errors will be bulked into your cancellation.
You also might want to set it so that right before WaitForFile is exited, it closes the file no matter what happens inside the method.
• The using statement should take care of closing (by calling Dispose()) the file. As far as I can tell there's no code path that does not do so. – millimoose Jan 8 '14 at 18:18