Concurrency problems with DD4T Binary Module

Question

If I clear the file system binary cache and request a binary from my (Tridion 2013SP1 ASP.NET MVC5 DD4T) web application with a number of concurrent users, I get errors related to the file being locked. Has anyone else experienced this, and if so found any resolution?

Typically there is first a message logged about problems writing the empty temp file:

IIS empty file could not be created.The process cannot access the file 'C:\inetpub\wwwroot\site\BinaryData\system\assets\css\main.css' because it is being used by another process

...and then an error when trying to transmit the file directly:

Exception occurred The process cannot access the file 'C:\inetpub\wwwroot\PerformanceWebsite\BinaryData\system\assets\css\main.css' because it is being used by another process

If I look at the file system while the test is running, I see the binary is created on the filesystem, but its filesize oscillates between 0kb and the full size (128kb in this case) which suggests that the lock in WriteBinaryToFile is not working...

Answer 1

DD4T uses the filesystem to cache binaries which are being retrieved from the Broker database. For this, it uses a HttpModule called BinaryDistributionModule, which checks each incoming URL to see whether it requests a binary. If a binary indeed is requested, a call to the BinaryFileManager class, which writes a fetched image to the filesystem if the publish date from the fetched binary is newer than the timestamp on the file.

The BinaryFileManager class is the class which causes the error you are experiencing. The likely cause of it lies in this bit of code in BinaryDistributionModule:

    #region private
    private static IBinaryFileManager _binaryFileManager = null;
    private static IBinaryFileManager BinaryFileManager 
    {
        get{
            if (_binaryFileManager == null)
                _binaryFileManager = UnityHelper.Container.Resolve<IBinaryFileManager>();
            return _binaryFileManager;
        }
    }
    #endregion

Here, it seems like there is only one (Singleton) instance of the BinaryFileManager class, but if you have more than one worker process, it will still fail if two processes are writing the same file.

Looking at the WriteBinaryToFile method, it seems there are some issues with it. First, the lock statement does a lock on a method variable instance. This does not lock multiple threads writing to the same file, so you'll have to do the lock with a static object on the class level.

This will not solve the problem of multiple processes writing to the same file though. For this, you will have to implement a file lock mutex mechanism, of which there are numerous examples to find if you follow the link. It might be that a FileStream.Lock in combination with a FileStream.CanRead for other processes is actually sufficient.

Update

I've been running some tests and the only solution without doing expensive mutex locking seems to be this:

        /// <summary>
    /// Perform actual write of binary content to file
    /// </summary>
    /// <param name="binary">The binary to store</param>
    /// <param name="physicalPath">String the file path to write to</param>
    /// <param name="dimensions">Dimensions of file</param>
    /// <returns>True is binary was written to disk, false otherwise</returns>
    private static bool WriteBinaryToFile(IBinary binary, String physicalPath, Dimensions dimensions)
    {
        if (!File.Exists(physicalPath))
        {
            FileInfo fileInfo = new FileInfo(physicalPath);
            if (fileInfo.Directory != null && !fileInfo.Directory.Exists)
            {
                fileInfo.Directory.Create();
            }
        }

        byte[] buffer = binary.BinaryData;

        if (dimensions != null && (dimensions.Width > 0 || dimensions.Height > 0))
        {
            buffer = ResizeImageFile(buffer, dimensions, GetImageFormat(physicalPath));
        }
        return TryWrite(physicalPath, buffer);
    }

    /// <summary>
    /// The only way to properly handle locked files if they are being written
    /// by a different process or thread. The sneaky bit here is that the IOException
    /// is caught, as there is no other way to properly process file locks
    /// without creating an expensive mutex.
    /// 
    /// Since there are multiple concurrent processes and threads trying to
    /// read the same file, they will now only be able read something when the process
    /// which is writing is done writing (as was previously the case anyway).
    /// As this happens in a matter of nanoseconds, that is no problem.
    /// </summary>
    /// <param name="path">The path to the file</param>
    /// <param name="buffer">The buffer to write to disk</param>
    private static bool TryWrite(string path, byte[] buffer)
    {
        try
        {
            // The using statement closes the filestream on dispose
            // and releases the file lock
            using (FileStream f = new FileStream(path, FileMode.Create, FileAccess.ReadWrite, FileShare.None))
            {
                f.Write(buffer, 0, buffer.Length);
                return true;
            }
        }
        catch (IOException)
        {
            Log.Warn("Cannot write to {0}. File is already locked by another process.",path);
        }
        return false;
    }

    /// <summary>
    /// For deleting files the same applies as for writing to it.
    /// If the file is locked somehow, then the request should just proceed
    /// and the next request (process or thread) should then handle the delete.
    /// </summary>
    /// <param name="physicalPath">The path of the file to delete</param>
    private static void DeleteFile(string physicalPath)
    {
        if (File.Exists(physicalPath))
        {
            Log.Debug("Requested binary {0} no longer exists in Broker. Removing...", physicalPath);

            try
            {
                File.Delete(physicalPath); // file got unpublished
            }
            catch (IOException)
            {
                Log.Warn("File '{0}' was in use. This can happen sporadically. Let the next thread handle this.",physicalPath);
            }
            Log.Debug("Done ({0})", physicalPath);
        }
    }

Answer 2

I don't have an answer (yet), probably more like additional information, although this can be used to form a final answer, so I'm posting it anyways. Hoping on some reactions from the community and possibly somebody who can answer it.

Looking at the DD4T code there are two classes dealing with binaries, BinaryDistributionModule.cs and BinaryFileManager.cs, and only in the BinaryFileManager class a lock is used in the WriteBinaryToFile() method. The lock used is a string, and while generally locking on strings is discouraged, because of deadlock situations, since only this single method is using that lock there seems to be a deeper issue.

Inspecting the rest of the code, there are more places that are dealing with (potentially) the same binary file, and it is done in both classes mentioned. Methods like BinaryFileManager.DeleteFile(), Binary.DistributionModule.DistributionModule_OnPreRequestHandlerExecute(), and BinaryDistributionModule.DistributionModule_OnBeginRequest() all should implement the same lock I suspect, or even better a different between read and write (multiple threads are allowed to read, as long as no other thread is trying to write at the same time, and only one thread is allowed to write the binary at all times).

I tried implementing a named locker using a ConcurrentDictionary on all mentioned methods, but that didn't seem to fix the problem. I still get exceptions while accessing the file, most are occurring on BinaryFileManager.WriteBinaryToFile(). So it appears that I'm still missing a method somewhere that accesses the binary in some way.

Here is the code of my named locker class:

internal static class NamedLocker
{
    private static readonly ConcurrentDictionary<string, object> Locks = new ConcurrentDictionary<string, object>();

    public static object GetLock(string name)
    {
        return Locks.GetOrAdd(name, s => new object());
    }

    public static void RemoveLock(string name)
    {
        object o;
        Locks.TryRemove(name, out o);
    }
}

And it is being used like this in BinaryFileManager.WriteBinaryToFile():

lock (NamedLocker.GetLock(physicalPath))
{
    if (File.Exists(physicalPath))
    {
        fileStream = new FileStream(physicalPath, FileMode.Create);
    }
    ...
    fileStream.Write(buffer, 0, buffer.Length);
}

And also in all other mentioned methods, like BinaryDistributionModule.DistributionModule_OnPreRequestHandlerExecute():

lock (NamedLocker.GetLock(request.PhysicalPath))
{
    response.TransmitFile(request.PhysicalPath);
}

As mentioned, I'm stuck here, there are still file access exceptions being thrown. Who can make us see the light?