Implementing a cancellable "infinite" loop

Question

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
 {
 Thread.Sleep(retryIntervalMillis);
 }
 }
 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, 
 FileAccess access = FileAccess.Read, 
 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 
 {
 Thread.Sleep(retryIntervalMillis);
 }
 }
 } 
 throw new OperationCanceledException(token);
}

• user33306: 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.
• millimoose: @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.
• Sam: 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?
• Bobby: Related: Waiting for a file to be accessible with Thread.Sleep() and exception catching.
• millimoose: @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.

Answer 1

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.

I would catch only the exceptions that you know about and are ready for, meaning that you have a solution for that exception.

---

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.

• millimoose: I've changed that part in my code since yesterday, I'll edit that into my sample. It's a valid observation about the code as posted, but really the crux of my question was, specifically, implementing cancellation correctly.
• millimoose: 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: I'm sorry you feel I was moving the goalposts with my edit, but I believe even as originally phrased it was clear what I'm asking about is how I implement cancellation, not the wait-and-retry mechanism itself.
• Malachi: on Code Review we Review what code you have. catching every error is bad practice, unless you are logging it. the new code looks better from what I have seen
• millimoose: Ah, I think I get it now. You weren't accusing me of moving the goalposts, but of messing up the context of your answer and making it seem like it's completely unrelated. Sorry for being argumentative there, I read the situation wrong.