Synchronising downloads and updates (2)

There was a potential deadlock in the last piece of code. When you obtain any kind of lock you will mostly use a try..finally block to ensure the lock is released when you have finished with it. The using() command expands to a try..finally block so the original code might look okay, however if you mentally expand the code you will see the problem.

using (FileLockingService.FileReadLock(product1.ID))
{
    //Some code here
}

This becomes

var disposable = FileLockingService.FileReadLock(product1.ID);
try
{
    //Some code here
}
finally
{
    disposable.Dispose();
}

So why is this a problem? In a single threaded application it isn't, but this is a website so there are multiple threads running at the same time. Now take into account the fact that a web request has a timeout, what if the thread is aborted after the lock is obtained? Ordinarily this isn't a problem because the acquisition is followed immediately by a "try", but if we expand this code further it is evident that this isn't the case for the code I originally wrote. The code below is indented per method rather than the normal logical indendation

• var disposable =
  
  • //Code inside FileLockingService
    
  • var lockHandler = GetLockHandler(int id);
    
  • lockHandler.EnterWriteLock();
    //The next command must be a try block
    
  • var enterWriteLockResult = new DisposableAction(..............); //Oops!
    
    • if (action == null) throw new ArgumentNullException("Action");
      
    • this.Action = action;
      
    • this.Context = context;
      
  • return enterWriteLockResult;
    
• try
  {
      //Some code here
  }
  finally
  {
  disposable.Dispose();
  }
  

So here you see that rather than there being a TRY block immediately after the EnterWriteLock the code above would create a DisposableAction, check a parameter, assign two instance members, and then return a result. If anything went wrong at any of these points (OutOfMemoryException, ThreadAbortException, etc) then we'd end up with a lock that would not get removed. The code has been changed as follows.

    public class FileLockingService : IFileLockingService
    {
        readonly object SyncRoot = new object();
        readonly Dictionary<int, WeakReference> Locks = new Dictionary<int, WeakReference>();

        public ReaderWriterLockSlim GetLockHandler(int productId)
        {
            WeakReference resultReference;
            ReaderWriterLockSlim result;
            lock (SyncRoot)
            {
                if (Locks.TryGetValue(productId, out resultReference))
                {
                    result = (ReaderWriterLockSlim)resultReference.Target;
                    if (result != null)
                        return result;
                }
                result = new ReaderWriterLockSlim();
                Locks[productId] = new WeakReference(result);
            }
            return result;
        }
    }

and when I use this service:

    FileLockingService.GetLockHandler(product.ID).EnterReadLock();
    //.NET wont allow any exceptions until flow passes into the try block
    try
    {
        //This is the earliest point that an exception can occur after acquiring the lock
        using (var sourceStream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read))
        {
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "Application/Binary");
            Response.AddHeader("Content-Disposition", "attachment; filename=" + product.ClientFileName);
            using (var resultStream = DownloadService.Process(currentUser, new List<string>(), product.ClientFileName, sourceStream))
            {
                int bytesRead;
                byte[] data = new byte[1024];
                while ((bytesRead = resultStream.Read(data, 0, data.Length)) > 0)
                    Response.OutputStream.Write(data, 0, bytesRead);
                Response.End();
            }//using
        }//filestream
    }
    finally
    {
        FileLockingService.GetLockHandler(product.ID).ExitReadLock();
    }

Lesson learned: When it comes to thread synchronisation don't do anything fancy!