Using threadpool with thread completion updates inside a lock

Question

Following several tutorials on threaded app development, I've implemented a small practice utility that uses both Task.Run, and Threadpool.QueueUserWorkItem to recursively index a folder and generate a checksum manifest of all contained files.

While the application works well, I'm unable to get the UI to update during the portion that specifically uses a threadpool. The UI does update correctly using await Application.Current.Dispatcher.BeginInvoke when using Task.Run.

The desired behavior is for the progress to continue to update within ProcessChecksums as each of the threadpool's threads finish their individual task.

int queuedThreads = 0;
object locker = new object();
CancellationTokenSource cancellationTokenSource;
List<string> lstFilePaths = new List<string>();
Manifest manifestData;
event Action<double> OnProgressChange;
event Action<string> OnStatusChange;
    
async void GenerateManifest(Object sender, RoutedEventArgs e) {
    status = "Indexing Files";
    progress = 1;

    cancellationTokenSource = new CancellationTokenSource();

    await Task.Run(() => Async_GenerateManifest(cancellationTokenSource.Token), cancellationTokenSource.Token);

    if (!cancellationTokenSource.Token.IsCancellationRequested) {
        Finished();
    }else{
        Cancelled();
    }
}
    
async Task Async_GenerateManifest(CancellationToken cancellationToken) {
    if (cancellationToken.IsCancellationRequested) { return; }

    Async_IndexFiles(initialpath, cancellationToken);
        
    //Works Perfectly
    await Application.Current.Dispatcher.BeginInvoke(DispatcherPriority.Background, new Action(() => {
        OnStatusChange("Generating Checksums");
        OnProgressChange(10);
    }));

    ProcessChecksums(cancellationToken);

    //Works Perfectly
    await Application.Current.Dispatcher.BeginInvoke(DispatcherPriority.Background, new Action(() => {
        OnStatusChange("Saving Manifest");
        OnProgressChange(90);
    }));

    SaveManifestFile(cancellationToken);
}
    
void ProcessChecksums(CancellationToken cancellationToken) {
    List<FileData> lstFileData = new List<FileData>();

    for (int i = 0; i < lstFilePaths.Count; i++) {
        if (cancellationToken.IsCancellationRequested) { return; }
            
        string s = lstFilePaths[i];
        lock (locker) queuedThreads++;

        ThreadPool.QueueUserWorkItem( x => {
            manifestData.AddFileData(new FileData(s, GenerateChecksum(s)));
        });
    }

    lock (locker) {
        while (queuedThreads > 0) {
            if (cancellationToken.IsCancellationRequested) { return; }=
            Monitor.Wait(locker);
                
            //Can't use await Dispatcher.BeginInvoke as is inside a lock, doesn't update GUI while waiting.
            OnProgressChange((((queuedThreads - lstFilePaths.Count) * -1) / lstFilePaths.Count) - 0.2);
        }
    }
}

string GenerateChecksum(string filePath) {
    //Time-consuming checksum generation
    //...

    lock (locker) {
        queuedThreads--;
        Monitor.Pulse(locker);
    }

    return BitConverter.ToString(checksum);
}

Answer 1

The standard pattern for updating the UI with progress updates from a background thread is to use IProgress<T>/Progress<T>. The more modern approach has several benefits over direct use of Dispatcher.

// You'd want to set these while on the UI thread.
// E.g., in your constructor:
//   _percentProgress = new Progress<double>(value => ...);
//   _statusProgress = new Progress<string>(value => ...);
IProgress<double> _percentProgress;
IProgress<string> _statusProgress;

async void GenerateManifest(Object sender, RoutedEventArgs e) {
  status = "Indexing Files";
  progress = 1;

  cancellationTokenSource = new CancellationTokenSource();

  await Task.Run(() => GenerateManifest(cancellationTokenSource.Token));

  if (!cancellationTokenSource.Token.IsCancellationRequested) {
    Finished();
  }else{
    Cancelled();
  }
}

void GenerateManifest(CancellationToken cancellationToken) {
  if (cancellationToken.IsCancellationRequested) { return; }

  Async_IndexFiles(initialpath, cancellationToken);

  _statusProgress?.Report("Generating Checksums");
  _percentProgress?.Report(10);

  ProcessChecksums(cancellationToken);

  _statusProgress?.Report("Saving Manifest");
  _percentProgress?.Report(90);

  SaveManifestFile(cancellationToken);
}
    
void ProcessChecksums(CancellationToken cancellationToken) {
  List<FileData> lstFileData = new List<FileData>();

  for (int i = 0; i < lstFilePaths.Count; i++) {
    if (cancellationToken.IsCancellationRequested) { return; }
            
    string s = lstFilePaths[i];
    lock (locker) queuedThreads++;

    ThreadPool.QueueUserWorkItem( x => {
        manifestData.AddFileData(new FileData(s, GenerateChecksum(s)));
    });
  }

  lock (locker) {
    while (queuedThreads > 0) {
      if (cancellationToken.IsCancellationRequested) { return; }
      Monitor.Wait(locker);

      _percentProgress?.Report((((queuedThreads - lstFilePaths.Count) * -1) / lstFilePaths.Count) - 0.2);
    }
  }
}

There are a few other problems with the code, too. QueueUserWorkItem should probably be replaced with Task.Run and an await Task.WhenAll to ensure exceptions don't tear down the process. The if (...) Finished(); else Cancelled(); is probably best represented by changing the return type to Task. Use ThrowIfCancellationRequested instead of IsCancellationRequested. And there's a kind of Monitor.Wait without a Pulse that is just used for progress updates, which is odd; it would be cleaner to allow that code to report its own progress. If you take a look at each of these individually, you should find your code ends up cleaner.