2

I need to perform few tasks inside a Windows Service I am writing in parallel. I am using VS2013, .NET 4.5 and this thread Basic design pattern for using TPL inside windows service for C# shows that TPL is the way to go.

Below is my implementation. I was wondering if anyone can tell me if I have done it correctly!

public partial class FtpLink : ServiceBase
{
    private readonly CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource();
    private readonly ManualResetEvent _runCompleteEvent = new ManualResetEvent(false);

    public FtpLink()
    {
        InitializeComponent();

        // Load configuration
        WebEnvironment.Instance.Initialise();
    }

    protected override void OnStart(string[] args)
    {
        Trace.TraceInformation("DatabaseToFtp is running");

        try
        {
            RunAsync(_cancellationTokenSource.Token).Wait();
        }
        finally
        {
            _runCompleteEvent.Set();
        }
    }

    protected override void OnStop()
    {
        Trace.TraceInformation("DatabaseToFtp is stopping");

        _cancellationTokenSource.Cancel();
        _runCompleteEvent.WaitOne();

        Trace.TraceInformation("DatabaseToFtp has stopped");
    }

    private async Task RunAsync(CancellationToken cancellationToken)
    {
        while (!cancellationToken.IsCancellationRequested)
        {
            Trace.TraceInformation("Working");

            // Do the actual work
            var tasks = new List<Task>
            {
                Task.Factory.StartNew(() => new Processor().ProcessMessageFiles(), cancellationToken),
                Task.Factory.StartNew(() => new Processor().ProcessFirmware(), cancellationToken)
            };

            Task.WaitAll(tasks.ToArray(), cancellationToken);

            // Delay the loop for a certain time
            await Task.Delay(WebEnvironment.Instance.DatabasePollInterval, cancellationToken);
        }
    }
}
Community
  • 1
  • 1
WRACK
  • 33
  • 6
  • 1
    This is better asked at http://codereview.stackexchange.com/ – CindyH Apr 09 '15 at 01:52
  • Unlike forum sites, we don't use "Thanks", or "Any help appreciated", or signatures on [so]. See "[Should 'Hi', 'thanks,' taglines, and salutations be removed from posts?](http://meta.stackexchange.com/questions/2950/should-hi-thanks-taglines-and-salutations-be-removed-from-posts). – John Saunders Apr 09 '15 at 04:36
  • Related: http://stackoverflow.com/q/25001764/1768303 – noseratio Apr 09 '15 at 18:35

1 Answers1

2

There are a few things i would do differently:

  1. OnStart should execute in a timely fashion. Common practice is to defer work to a background thread which is in charge of doing the actual work. You're actually doing that but blocking that thread with a call to Task.Wait, which kind of makes the offloading to a background thread useless, because execution becomes synchronous again.
  2. You're using the sync over async anti-pattern, this should be mostly avoided. Let the calling method invoke the work in parallel.
  3. I think you might be using the ManualResetEvent the other way around. You're wrapping your RunAsync method in a try-finally block, but you're only calling WaitOne from OnStop. I'm not really sure you need a lock here at all, it doesn't seem (from your current code) that this code is being invoked in parallel. Instead, you can store the Task returned by RunAsync in a field and wait on it to complete.
  4. You're using the blocking version, WaitAll. Instead, you could use the asynchronous version, Task.WhenAll, which can be asynchronously waited.
Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
  • Thanks. I am still learning a lot about TPL. I extracted the code from the Azure WorkerRole template provided by Microsoft. If you can help me a little more with necessary changes, I would really appreciate it. That being said, the first thing I need to change is `RunAsync(_cancellationTokenSource.Token).Wait();` to 'Task.Run(() => StartProcessing(_cancellationTokenSource.Token));' and then change the signature of method `private async Task RunAsync(CancellationToken cancellationToken) ` to `private void StartProcessing(CancellationToken cancellationToken)` – WRACK Apr 09 '15 at 04:14
  • Azure workers and windows services are different in the sense that worker roles must not exit for the lifetime of the service, while the windows service is supposed to return rather quickly. Ill try to add a code snippet tomorrow for an implementation i would use for a service, hope that'll get you on the right path. – Yuval Itzchakov Apr 09 '15 at 04:17