C# - Locking issues with Mutex

Question

I have a WebAPI in C#, and I need to load a Document by POST, modify some parameters, and save it to an Azure Blob account.

I can read the file, I can modify the parameters, but when I save it to Azure, the document is saved as the original file.

/* file es del tipo System.Net.Http.StreamContent que viene en un POST */

byte[] byteArray = await file.ReadAsByteArrayAsync();
MemoryStream docStream = new MemoryStream();

docStream.Write(byteArray, 0, byteArray.Length);
docStream.Seek(0x00000000, SeekOrigin.Begin);

WordprocessingDocument doc = WordprocessingDocument.Open(docStream, true);
MainDocumentPart mainDocumentPart = doc.MainDocumentPart;

var descedants = mainDocumentPart.Document.Body.Descendants<SdtElement>();

var idRef = descedants.Where(c => c.FirstChild.Elements<SdtAlias>().Where(v => v.Val == "Asunto").Count() > 0).FirstOrDefault();
if (idRef != null && user != null)
{
    var tNomRef = idRef.Elements<SdtContentRun>().FirstOrDefault().FirstOrDefault();
    var textNomRef = (DocumentFormat.OpenXml.OpenXmlLeafTextElement)tNomRef.LastOrDefault();
    textNomRef.Text = info.idDocumento;
}

var titRef = descedants.Where(c => c.FirstChild.Elements<SdtAlias>().Where(v => v.Val == "Título").Count() > 0).FirstOrDefault();
if (titRef != null && user != null)
{
    var tNomRef = idRef.Elements<SdtContentRun>().FirstOrDefault().FirstOrDefault();
    var textNomRef = (DocumentFormat.OpenXml.OpenXmlLeafTextElement)tNomRef.LastOrDefault();
    textNomRef.Text = info.nomDocumento;
}

/* Este objeto se encarga de subir el archivo a un Blob Storage de Azure */
CloudBlockBlob blob = imagesContainer.GetBlockBlobReference(xNombre);
docStream.Position = 0;
blob.UploadFromStream(docStream);
docStream.Close();
docStream.Dispose();

Solution 1

You should only be using Mutexes if you need cross-process synchronization.

Although a mutex can be used for intra-process thread synchronization, using Monitor is generally preferred, because monitors were designed specifically for the .NET Framework and therefore make better use of resources. In contrast, the Mutex class is a wrapper to a Win32 construct. While it is more powerful than a monitor, a mutex requires interop transitions that are more computationally expensive than those required by the Monitor class.

If you need to support inter-process locking you need a Global mutex.

The pattern being used is incredibly fragile, there is no exception handling and you are not ensuring that your Mutex is released. That is really risky code and most likely the reason you see these hangs when there is no timeout.

Also, if your file operation ever takes longer than 1.5 seconds then there is a chance concurrent Mutexes will not be able to grab it. I would recommend getting the locking right and avoiding the timeout.

I think its best to re-write this to use a lock. Also, it looks like you are calling out to another method, if this take forever, the lock will be held forever. That's pretty risky.

This is both shorter and much safer:

// if you want timeout support use 
// try{var success=Monitor.TryEnter(m_syncObj, 2000);}
// finally{Monitor.Exit(m_syncObj)}
lock(m_syncObj)
{
    l.LogInformation( "Got lock to read/write file-based server state."
                    , (Int32)VipEvent.GotStateLock);
    using (var fileStream = File.Open( ServerState.PATH, FileMode.OpenOrCreate
                                     , FileAccess.ReadWrite, FileShare.None))
    {
        // the line below is risky, what will happen if the call to invoke
        // never returns? 
        result = func.Invoke(fileStream);
    }
}

l.LogInformation("Released state file lock.", (Int32)VipEvent.ReleasedStateLock);
return true;

// note exceptions may leak out of this method. either handle them here.
// or in the calling method. 
// For example the file access may fail of func.Invoke may fail

Solution 2

If some of the file operations fail, the lock will not be released. Most probably that is the case. Put the file operations in try/catch block, and release the lock in the finally block.

Anyway, if you read the file in your Global.asax Application_Start method, this will ensure that noone else is working on it (you said that the file is read on application start, right?). To avoid collisions on application pool restaring, etc., you just can try to read the file (assuming that the write operation takes an exclusive lock), and then wait 1 second and retry if exception is thrown.

Now, you have the problem of synchronizing the writes. Whatever method decides to change the file should take care to not invoke a write operation if another one is in progress with simple lock statement.

Comments

[halfer]

Please read Under what circumstances may I add “urgent” or other similar phrases to my question, in order to obtain faster answers? - the summary is that this is not an ideal way to address volunteers, and is probably counterproductive to obtaining answers. Please refrain from adding this to your questions.

[Juan Ríos]

OK, thanks for your time and explanations. I will do this in the future. Best regards.

[Daniel Schaffer]

"if your file operation ever takes longer than 1.5 seconds then the next Mutex will not be able to grab it." - Are you sure? I would think that any request started less than 1.5 seconds before it finished would fail, but not otherwise.

[Daniel Schaffer]

I switched to using Monitor instead of a Mutex, and I'm not having the issue any more. Thanks!

[Juan Ríos]

Hi SLaks, thanks for your response, but how I can do it? I try creating a new MemoryStream, but when I save the documento into this, the MemoryStream closes, and I can´t read it or upload it to Azure.

[SLaks]

@JuanRíos: Use ToArray().

[Juan Ríos]

I think I don't understand, if I convert the actual MemoryStream to Byte Array, the file is the same. What thing will I be leaving out?

[Juan Ríos]

Many thanks for your comment. I add this to my code, and finally it works.

[Juan Ríos]

doc.Save(); byte[] newArray = docStream.ToArray(); blob.UploadFromByteArray(newArray, 0, newArray.Length);