HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Implementing a cancellable "infinite" loop

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cancellableloopinfiniteimplementing

Problem

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 action = null,
int retryIntervalMillis = 200)
{
while (!token.IsCancellationRequested)
{
try
{
va

Solution

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.

Context

StackExchange Code Review Q#38807, answer score: 10

Revisions (0)

No revisions yet.