3
\$\begingroup\$

I'm trying to poll for newly created files.

Because the FileSystemWatcher class is known not to be reliable (here, here and here), and because it does not wait for the end of a complete file write, I created a new class to get files creation.

The specifications are:

  • Files should be signaled only when the file write is complete (large files should be signaled at the end of the copy)
  • Files should be signaled only once
  • Existing files (when the program starts) should be signaled
  • The poller will live in a windows service. Thus it should support very long lifetime (several weeks)

Here's what I have right now:

public class FileSystemPoller
{
    private readonly string _path;
    private readonly CancellationToken _cancellationToken;
    private readonly string _searchPattern;
    private readonly SearchOption _searchOptions;
    private readonly HashSet<string> _filesProcessed = new HashSet<string>();
    private readonly TimeSpan _interval;

    public FileSystemPoller(
        string path,
        CancellationToken cancellationToken,
        string searchPattern = null,
        SearchOption options = SearchOption.TopDirectoryOnly,
        TimeSpan? interval = null
        )
    {
        _path = path;
        _cancellationToken = cancellationToken;
        _searchPattern = searchPattern ?? "*";
        _searchOptions = options;
        _interval = interval.GetValueOrDefault(TimeSpan.FromSeconds(1));
    }

    private async Task PollForChanges(BlockingCollection<FileInfo> outQueue)
    {
        var di = new DirectoryInfo(_path);

        var files = di.GetFiles(_searchPattern, _searchOptions);

        foreach (var file in files)
        {
            if (!_filesProcessed.Contains(file.FullName) && !IsFileLocked(file))
            {
                Debug.WriteLine($"File created disk : {file.FullName}");
                outQueue.Add(file);
                _filesProcessed.Add(file.FullName);
            }
        }
        foreach (var filePath in _filesProcessed)
        {
            if (!File.Exists(filePath))
            {
                _filesProcessed.Remove(filePath);
                Debug.WriteLine($"Removed from disk : {filePath}");
            }
        }

        Debug.WriteLine("Poll directory changes");

        await Task.Delay(_interval).ConfigureAwait(false);
        await PollForChanges(outQueue).ConfigureAwait(false);
    }

    public IEnumerable<FileInfo> GetCreatedFiles()
    {
        if (_cancellationToken.IsCancellationRequested) yield break;

        using (var queue = new BlockingCollection<FileInfo>())
        {
            PollForChanges(queue);

            if (!_cancellationToken.IsCancellationRequested)
            {
                foreach (var fileInfo in queue.GetConsumingEnumerable(_cancellationToken))
                {
                    yield return fileInfo;
                }
            }
        }
    }

    private static bool IsFileLocked(FileInfo file)
    {
        FileStream stream = null;

        try
        {
            stream = file.Open(FileMode.Open,
                     FileAccess.ReadWrite, FileShare.None);
        }
        catch (IOException)
        {
            //the file is unavailable because it is:
            //still being written to
            //or being processed by another thread
            //or does not exist (has already been processed)
            Debug.WriteLine($"File present but locked : {file.FullName}");

            return true;
        }
        finally
        {
            stream?.Close();
        }

        //file is not locked
        return false;
    }
}

This code seems to work, but:

Globally, what are possible improvements?

\$\endgroup\$
  • \$\begingroup\$ Please do not add, remove, or edit code in a question after you've received an answer. The site policy is explained in What to do when someone answers. \$\endgroup\$ – 409_Conflict Jul 4 '18 at 13:31
  • 1
    \$\begingroup\$ Is FileSystemWatcher “not known to be reliable” or “known not to be reliable”? Is that a “commonly known” fact or can you provide some sources? \$\endgroup\$ – Martin R Jul 4 '18 at 13:42
  • 2
    \$\begingroup\$ @MartinR, sorry, I'm not native english speaker, the difference is very subtile for me. Anyway, I've added some links to point to user complaining the FSW is not reliable, especiaaly when using network file shares \$\endgroup\$ – Steve B Jul 4 '18 at 13:47
  • \$\begingroup\$ FileSystemWatcher is "known to be unreliable". In simple words, it doesn't work well. If the data size of the collected watcher events exceeds the size of the buffer created by FileSystemWatcher, the events that overflow are lost, and reliable exceptions are not raised. There is plentiful discussion of these issues if one googles for them. Here is but one: stackoverflow.com/questions/239988/… \$\endgroup\$ – spinjector Apr 23 at 15:14
3
\$\begingroup\$

There is something wrong in the way you are using the cancellation token:

In the code GetConsumingEnumerable() throws if the cancellation token is set:

        foreach (var fileInfo in queue.GetConsumingEnumerable(_cancellationToken))
        {
          yield return fileInfo;
        }

If you don't handle that exception the application crashes here. You can't handle the exception here with a surrounding try-catch because yield doesn't allow that.

If the caller handles it, the application doesn't crash, but the recursive PollForChanges() continues calling itself <=> the application continues to run/doesn't stop properly?

Therefore you should check the cancellation token in PollForChanges() too:

  private async Task PollForChanges(BlockingCollection<FileInfo> outQueue)
  {
    if (_cancellationToken.IsCancellationRequested) return;
    ....

This DirectoryInfo:

var di = new DirectoryInfo(_path);

should be a class member instead of a local variable, because it doesn't change per instance. It can then replace _path.


IsFileLocked() can be simplified with a using statement:

      using (FileStream stream = file.Open(FileMode.Open, FileAccess.ReadWrite, FileShare.None))
      {

      }

instead of the finally statement.


This foreach-statement will crash if a filePath is actually removed by it:

  foreach (var filePath in _filesProcessed)
  {
    if (!File.Exists(filePath))
    {
      _filesProcessed.Remove(filePath);
      Debug.WriteLine($"Removed from disk : {filePath}");
    }
  }

I don't like the recursive approach for a long running service because it builds up the stack. I would find an iterative solution instead. Beside that I don't think I understand the async-await approach.

I would go with a simpler approach like something below:

public class FileSystemPoller
{
  private readonly CancellationToken _cancellationToken;
  private readonly string _searchPattern;
  private readonly SearchOption _searchOptions;
  private readonly HashSet<string> _filesProcessed = new HashSet<string>();
  private readonly TimeSpan _interval;
  private readonly DirectoryInfo _di;

  public FileSystemPoller(
      string path,
      CancellationToken cancellationToken,
      string searchPattern = null,
      SearchOption options = SearchOption.TopDirectoryOnly,
      TimeSpan? interval = null
      )
  {
    _cancellationToken = cancellationToken;
    _searchPattern = searchPattern ?? "*";
    _searchOptions = options;
    _interval = interval.GetValueOrDefault(TimeSpan.FromSeconds(1));
    _di = new DirectoryInfo(path);
  }

  public IEnumerable<FileInfo> GetCreatedFiles()
  {
    while (!_cancellationToken.IsCancellationRequested)
    {
      var files = _di.GetFiles(_searchPattern, _searchOptions);

      foreach (var file in files)
      {
        if (!_filesProcessed.Contains(file.FullName) && !IsFileLocked(file))
        {
          Console.WriteLine($"File created disk : {file.FullName}");
          _filesProcessed.Add(file.FullName);
          yield return file;
        }
      }

      _filesProcessed.RemoveWhere((p) => {
        if (!File.Exists(p))
        {
          Console.WriteLine($"Removed from disk : {p}");
          return true;
        }
        return false;
      });

      Console.WriteLine("Poll directory changes");
      Thread.Sleep(_interval);
    }
  }

  private static bool IsFileLocked(FileInfo file)
  {
    try
    {
      using (FileStream stream = file.Open(FileMode.Open, FileAccess.ReadWrite, FileShare.None))
      {

      }
    }
    catch (IOException)
    {
      //the file is unavailable because it is:
      //still being written to
      //or being processed by another thread
      //or does not exist (has already been processed)
      Console.WriteLine($"File present but locked : {file.FullName}");

      return true;
    }

    //file is not locked
    return false;
  }

}

With a test client like:

using (CancellationTokenSource tokenSource = new CancellationTokenSource())
{

  CancellationToken token = tokenSource.Token;

  ThreadPool.QueueUserWorkItem((obj) =>
  {
    try
    {

      FileSystemPoller poller = new FileSystemPoller(@"<Path>", token);

      foreach (var fi in poller.GetCreatedFiles())
      {
        // TODO: Handle each file info...
        Console.WriteLine(fi.FullName);
      }
    }
    catch (Exception ex)
    {
      Console.WriteLine(ex);
    }
  });

  Console.ReadLine();

  tokenSource.Cancel();
}


Another thing: have you considered to use the FileInfo.CreationTime/LastWriteTime etc. to track changes instead of holding FileInfos in memory?

\$\endgroup\$
  • 1
    \$\begingroup\$ Very detailled answer. Thank you very much. Some remarks: you were right, removing items from the collection within its enumeration work crash. Thanks to RemoveWhere, it is easy to solve Regarding the scheduling, I choose to follow stackoverflow.com/a/23814733/588868, which does not stack. Finally, I removed the use of the FileInfo class, in profit a simple string that holds the file path. This is simpler and avoid loading a load of data. The consumer of the class will be responsible to process the file. \$\endgroup\$ – Steve B Jul 4 '18 at 12:55
  • \$\begingroup\$ Including part of your changes : gist.github.com/stevebeauge/088603022062a683929abb6784451e4d \$\endgroup\$ – Steve B Jul 4 '18 at 13:42
  • \$\begingroup\$ Thread.Sleep - this is not good at all. Why not use a timer? \$\endgroup\$ – t3chb0t Jul 4 '18 at 15:06
  • \$\begingroup\$ Is there a reason why the Debug.WriteLines turned into Console.WriteLines, or is that just oversight? \$\endgroup\$ – VisualMelon Jul 4 '18 at 19:10
  • \$\begingroup\$ @VisualMelon: I just did some tests in a Console app :-) \$\endgroup\$ – Henrik Hansen Jul 4 '18 at 19:45
2
\$\begingroup\$

Bug

Your code is broken in GetCreatedFiles because you don't await the result of PollForChanges. But I guess this is intended as you cannot use await in an iterator. I still consider it a bug because it can happen that you start enumerating the queue before soemthing was added to it. In case of an exception there might be a delay so you won't get any results from it.

Code

I don't agree with you calling it a poller because it's not what it does. It's an enumerator with a delay; we can see that here:

public IEnumerable<FileInfo> GetCreatedFiles()

The actual poller is the user of this class. In order to make it a poller you'd have to make it run without being triggered by the user.

You have two options to achieve this:

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.