is this correct use of mutex to avoid concurrent modification to file?

Question

i want to be able to put the code of writing file into mutex so as to avoid any concurrent modification to a file. however, I want only the file with particular name be blocked as critical section not for the other file. will this code work as expected or have I missed anything?

private async static void StoreToFileAsync(string filename, object data)
    {
        IsolatedStorageFile AppIsolatedStorage = IsolatedStorageFile.GetUserStoreForApplication();
        if (IsolatedStorageFileExist(filename))
        {
            AppIsolatedStorage.DeleteFile(filename);
        }
        if (data != null)
        {
            string json = await Task.Factory.StartNew<string>(() => JsonConvert.SerializeObject(data));
            if (!string.IsNullOrWhiteSpace(json))
            {
                byte[] buffer = Encoding.UTF8.GetBytes(json);
                Mutex mutex = new Mutex(false, filename);
                mutex.WaitOne();
                IsolatedStorageFileStream ISFileStream = AppIsolatedStorage.CreateFile(filename);
                await ISFileStream.WriteAsync(buffer, 0, buffer.Length);
                ISFileStream.Close();
                mutex.ReleaseMutex();
            }
        }
    }

EDIT 1: or should I replace the async write with synchronous one and run as a separate task?

await Task.Factory.StartNew(() =>
                    {
                        if (!string.IsNullOrWhiteSpace(json))
                        {
                            byte[] buffer = Encoding.UTF8.GetBytes(json);
                            lock (filename)
                            {
                                IsolatedStorageFileStream ISFileStream = AppIsolatedStorage.CreateFile(filename);
                                ISFileStream.Write(buffer, 0, buffer.Length);
                                ISFileStream.Close();
                            }
                        }
                    });

I hope this will work. Mutex is costly as it is kernal object. You can use criticalsection when only one process (With different thread) access the file. — Sivaraman, Apr 15 '14 at 09:30
@Sivaraman can u check (edit1) whether the criticalsection implementation is correct? — Ankit, Apr 15 '14 at 10:03
Might have been a better fit on http://codereview.stackexchange.com/ — WiredPrairie, Apr 15 '14 at 11:03

score 3 · Answer 1 · answered Apr 15 '14 at 09:44

3

In the general case, using a Mutex with asynchronous code is wrong. Normally, I'd say you should use something like AsyncLock from my AsyncEx library, and if you want a separate lock per file, then you'll need a dictionary or some such.

However, if your method is always called from the UI thread, then it will work. It is not very efficient, but it will work.

answered Apr 15 '14 at 09:44

Stephen Cleary

437,863
77
675
810

Isnt there exists some kind of async mutex? I assume, having mutex with file name (+ some uid) would solve the problem without dictionary, isnt it? – Vitalii Vasylenko May 10 '14 at 22:11
There is not an asynchronous *named* mutex, no. – Stephen Cleary May 11 '14 at 01:08

score 2 · Answer 2 · edited May 23 '17 at 10:32

There are few things you should improve:

running asynchronous code 'inside' Mutex is not good, as the same thread should create and release Mutex. You will probably get exception when called not from UI thread.
filename is not a good name for a Mutex - it is global object (among all processes), so it should be unique.
you should Dispose your Mutex, and Relese it in finally clause (what happens if you get an Exception?)- BTW here is a good pattern
you should/must Dispose IsolatedFileStream
you should also think if infinite waiting is a good choice
if you aren't accessing to file amopng processes (Bacground Agents, other App) then you can use lock or SemaphoreSlim

private async static void StoreToFileAsync(string filename, object data)
{
    using (IsolatedStorageFile AppIsolatedStorage = IsolatedStorageFile.GetUserStoreForApplication())
    {
        if (IsolatedStorageFileExist(filename))
            AppIsolatedStorage.DeleteFile(filename);
        if (data != null)
        {
            string json = await Task.Factory.StartNew<string>(() => JsonConvert.SerializeObject(data));
            if (!string.IsNullOrWhiteSpace(json)) await Task.Run(() =>
                {
                    byte[] buffer = Encoding.UTF8.GetBytes(json);
                    using (Mutex mutex = new Mutex(false, filename))
                    {
                        try
                        {
                            mutex.WaitOne();
                            using (IsolatedStorageFileStream ISFileStream = AppIsolatedStorage.CreateFile(filename))
                                ISFileStream.Write(buffer, 0, buffer.Length);
                        }
                        catch { }
                        finally { mutex.ReleaseMutex(); }
                    }
                });
        }
    }
}

EDIT: Removed aynchronous call from Mutex - as it will cause problems when called not from UI thread.

i cant use lock on awaited operation, so is it okay if replace the **WriteAsync** with a synchronous **Write** and run that as a separate Task? — Ankit, Apr 15 '14 at 09:54
plz have a look at the edit 1 in the question. btw why shouldn't I use lock on file name? — Ankit, Apr 15 '14 at 09:59
I think it should work. You are just awaiting Writing so with Write run on asynchronous thread it should be the same. As for naming lock - [follow the remarks here](http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx). I wrote that naming Mutex is not a good choice, for example think that you have same files in different folders. You can also think about waht Stephen Cleary had written with AsyncLock. — Romasz, Apr 15 '14 at 10:11
ok, so if I am sure I have just one file with that name in all folders, shouldn't be any problem? — Ankit, Apr 15 '14 at 10:14
Better would be to create a `Dictionary` with locks. And put your `IsolatedFileStream` into `using` or `Dispose()` it at the end. — Romasz, Apr 15 '14 at 10:18

is this correct use of mutex to avoid concurrent modification to file?

2 Answers2