53

I have some problems with ReaderWriterLockSlim. I cannot understand how it's magic working.

My code:

 private async Task LoadIndex()
    {
        if (!File.Exists(FileName + ".index.txt"))
        {
            return;
        }
        _indexLock.EnterWriteLock();// <1>
        _index.Clear();
        using (TextReader index = File.OpenText(FileName + ".index.txt"))
        {
            string s;
            while (null != (s = await index.ReadLineAsync()))
            {
                var ss = s.Split(':');
                _index.Add(ss[0], Convert.ToInt64(ss[1]));
            }
        }
        _indexLock.ExitWriteLock();<2>
    }

When I enter write lock at <1>, in debugger I can see that _indexLock.IsWriteLockHeld is true, but when execution steps to <2> I see _indexLock.IsWriteLockHeld is false and _indexLock.ExitWriteLock throws an exception SynchronizationLockException with message "The write lock is being released without being held". What I doing wrong?

Adriano Repetti
  • 65,416
  • 20
  • 137
  • 208
l0nley
  • 611
  • 1
  • 5
  • 9
  • How is `_indexLock` initialised? Could another thread be initialising it at the same time as a different thread is in `LoadIndex()`? – Matthew Watson Oct 29 '13 at 13:24
  • maybe another thread that has access to _indexLock is reinitializing it.... it couldn't be released by another thread for sure, but maybe reinitialized to a new instance all together... – Sadek Noureddine Oct 29 '13 at 13:27
  • It doesn't require a thread to get _indexLock to be overwritten. – Hans Passant Oct 29 '13 at 13:33
  • _indexLock is initialized when class istance created and declared like this: private readonly ReaderWriterLockSlim _indexLock = new ReaderWriterLockSlim(). There are no one thread can modify it. – l0nley Oct 29 '13 at 15:08

5 Answers5

87

ReaderWriterLockSlim is a thread-affine lock type, so it usually cannot be used with async and await.

You should either use SemaphoreSlim with WaitAsync, or (if you really need a reader/writer lock), use my AsyncReaderWriterLock from AsyncEx or Stephen Toub's AsyncReaderWriterLock.

Pang
  • 9,564
  • 146
  • 81
  • 122
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • 1
    Wouldn't be also correct to say that it depends on the platform? The code posted by OP doesn't mention if it's WPF or a console app. In a console app it's not working because there's no `SynchronizationContext` there. The code works ok in a WPF app. http://stackoverflow.com/questions/15882710/is-it-safe-to-use-readerwriterlockslim-in-an-async-method?noredirect=1&lq=1 – Don Box May 17 '17 at 09:18
  • @DonBox: No; as I noted in my answer to that question, even if you resume on the same thread, you're still allowing arbitrary code to run while you're "holding" that lock. – Stephen Cleary May 17 '17 at 13:33
  • Oh man, and I thought I got it right. Can you explain what you mean by "arbitrary code"? You mean that code is not working properly not even in WPF? What's wrong with holding the lock there? In the answer, what did you mean by "usually" "it usually cannot be used"? Thanks! – Don Box May 17 '17 at 18:16
  • 1
    @DonBox: You've been asking a lot of questions in comments. I think you would benefit from asking your own question. – Stephen Cleary May 17 '17 at 21:07
  • 1
    OK, makes sense, here it is http://stackoverflow.com/questions/44036160/readerwriterlockslim-questions – Don Box May 17 '17 at 23:11
  • With some experimentation, I've noted that when calling an async method with await (await foo()), the thread that enters the method is generally NOT the same thread that exits the method. This creates problems for all of C#'s locks that have thread affinity. – StvnBrkdll Feb 16 '21 at 17:36
  • i wonder if through semaphoreslim one can deal with dirty read problems ? Like if i do not want my threads to read at the same time when a write is happening, but at the same time i want to allow multiple threads to be able to read at same time. Thanks – kuldeep Jan 27 '23 at 08:22
  • @kuldeep: The vast, vast majority of the time RWL semantics are unnecessary and a simple lock will suffice. – Stephen Cleary Jan 27 '23 at 13:14
  • so in case of where i am using IMemoryCache having read only threads also wait on semaphore slim is ok to as long as the app is not dealing with very high traffic and one just wants to not hit the DB for every single request, and rather simply use cache. The changes in data are relatively quite low in single digit % wise compare to read access. I was taking a look at your library AsyncEx but i am happy with semaphore slim if this is not a performance impact (which i dont know yet becasue i have not done any benchmarking) – kuldeep Feb 02 '23 at 12:28
  • 1
    @kuldeep: I would definitely use `SemaphoreSlim` myself unless there was a clear reason not to based on real-world metrics. – Stephen Cleary Feb 02 '23 at 15:02
6

You can safely emulate a reader/writer locking mechanism using the reliable and lightweight SemaphoreSlim and keep the benefits of async/await. Create the SemaphoreSlim giving it the number of available locks equivalent to the number of routines that will lock your resource for reading simultaneously. Each one will request one lock as usual. For your writing routine, make sure it requests all the available locks before doing its thing.

That way, your writing routine will always run alone while your reading routines might share the resource only between themselves.

For example, suppose you have 2 reading routines and 1 writing routine.

SemaphoreSlim semaphore = new SemaphoreSlim(2);

async void Reader1()
{
    await semaphore.WaitAsync();
    try
    {
        // ... reading stuff ...
    }
    finally
    {
        semaphore.Release();
    }
}

async void Reader2()
{
    await semaphore.WaitAsync();
    try
    {
        // ... reading other stuff ...
    }
    finally
    {
        semaphore.Release();
    }
}

async void ExclusiveWriter()
{
    // the exclusive writer must request all locks
    // to make sure the readers don't have any of them
    // (I wish we could specify the number of locks
    // instead of spamming multiple calls!)
    await semaphore.WaitAsync();
    await semaphore.WaitAsync();
    try
    {
        // ... writing stuff ...
    }
    finally
    {
        // release all locks here
        semaphore.Release(2);
        // (oh here we don't need multiple calls, how about that)
    }
}

Obviously this method only works if you know beforehand how many reading routines you could have running at the same time. Admittedly, too much of them would make this code very ugly.

mycelo
  • 409
  • 5
  • 4
  • 4
    This isn't providing read/write semantics. The idea of a reader/writer lock is that there can be any number of concurrent readers as you want, but when there is a write operation going on there cannot be any readers or other writers. This both unnecessarily limits readers, and doesn't properly limit readers or other writers when one operation is writing. – Servy Jun 29 '15 at 17:08
  • 2
    I does block any writers when you have active readers, and it still does allow concurrent readers when you don't have any active writers. That's essentially what a reader/writer lock should do, except when you don't know how many readers you'd have, as I **clearly** pointed out (even that could be got around if you really think about it, but I wouldn't spoil the surprise for you). It also might not be so elegant for your standards, but it **is** a valid solution and I myself used that in well-tested well-stressed high demand services. I'm all about going simple, but each to its own... – mycelo Jun 30 '15 at 11:13
  • 1
    This isn't bad for a quick solution that doesn't rely on external dependencies. Should probably be wrapped off into it's own class for reuse if needed (calling multiple `.WaitAsync()` for every read, plus remembering how many readers you accounted for on every use, and changing that across every use when necessary, would be ugly and bug-prone). – Marc L. Dec 15 '17 at 21:37
  • 16
    Looking at this a few months later: isn't there a possibility of deadlock if two threads call into `ExclusiveWriter` simultaneously? If both make it past the first `WaitAsync()` call, the call count will increase to two and both will be waiting for a `Release()`. – Marc L. Mar 09 '18 at 18:24
  • 2
    @MarcL. I've wrapped the exclusive write method in its own writeLock semaphore. Only allowing a single thread inside. – CodeMonkey Feb 10 '20 at 13:57
5

Some time ago I implemented for my project class AsyncReaderWriterLock based on two SemaphoreSlim. Hope it can help. It is implemented the same logic (Multiple Readers and Single Writer) and at the same time support async/await pattern. Definitely, it does not support recursion and has no protection from incorrect usage:

var rwLock = new AsyncReaderWriterLock();

await rwLock.AcquireReaderLock();
try
{
    // ... reading ...
}
finally
{
    rwLock.ReleaseReaderLock();
}

await rwLock.AcquireWriterLock();
try
{
    // ... writing ...
}
finally
{
    rwLock.ReleaseWriterLock();
}


public sealed class AsyncReaderWriterLock : IDisposable
{
    private readonly SemaphoreSlim _readSemaphore  = new SemaphoreSlim(1, 1);
    private readonly SemaphoreSlim _writeSemaphore = new SemaphoreSlim(1, 1);
    private          int           _readerCount;

    public async Task AcquireWriterLock(CancellationToken token = default)
    {
        await _writeSemaphore.WaitAsync(token).ConfigureAwait(false);
        await SafeAcquireReadSemaphore(token).ConfigureAwait(false);
    }

    public void ReleaseWriterLock()
    {
        _readSemaphore.Release();
        _writeSemaphore.Release();
    }

    public async Task AcquireReaderLock(CancellationToken token = default)
    {
        await _writeSemaphore.WaitAsync(token).ConfigureAwait(false);

        if (Interlocked.Increment(ref _readerCount) == 1)
        {
            try
            {
                await SafeAcquireReadSemaphore(token).ConfigureAwait(false);
            }
            catch
            {
                Interlocked.Decrement(ref _readerCount);

                throw;
            }
        }

        _writeSemaphore.Release();
    }

    public void ReleaseReaderLock()
    {
        if (Interlocked.Decrement(ref _readerCount) == 0)
        {
            _readSemaphore.Release();
        }
    }

    private async Task SafeAcquireReadSemaphore(CancellationToken token)
    {
        try
        {
            await _readSemaphore.WaitAsync(token).ConfigureAwait(false);
        }
        catch
        {
            _writeSemaphore.Release();

            throw;
        }
    }

    public void Dispose()
    {
        _writeSemaphore.Dispose();
        _readSemaphore.Dispose();
    }
}
xtadex
  • 96
  • 1
  • 4
  • Something doesn't seem right. The `AcquireReaderLock` acquires the `_writeSemaphore`, so probably the readers will be serialized (no concurrent readers allowed). Btw I think that the `SafeAcquire...` methods would be redundant if you had utilized `try`/`finally` blocks inside the other methods. – Theodor Zoulias Nov 09 '20 at 19:09
  • 2
    @TheodorZoulias, You right AcquireReaderLock acquire firstly _writeSemaphore to make sure no any writers in play at this moment. Write semaphore released immediately once _readerLock acquired. If you noticed, Read/Write acquire methods wait for both semaphores. So SafeAcquireReadSemaphore() used for cases when a cancellation happened in second WaitAsync() to properly release resources in case of OCE. – xtadex Nov 10 '20 at 23:32
  • Oh, I got it now. The logic seems solid, upvoted. I suggest to explain the logic a bit more inside the answer itself, so that future users of the code can use it with more confidence. – Theodor Zoulias Nov 11 '20 at 06:49
  • 1
    @xtadex one thing I noticed is that inside AcquireReaderLock an exception after the increment will leave the _readerCount as it was before. At least when using the pattern in the answer, the caller will not call release so there will always be that one reader that was aborted. One way it could happen is if the token gets cancelled . – eglasius Jul 02 '21 at 15:17
  • 1
    Good catch @eglasius. I went through code again in almost 100% (except for some tricky race conditions) _readSemaphore state is free and waiting never happened there. But I've added try/catch to handle that case. Thanks – xtadex Jul 03 '21 at 23:55
  • @xtadex yes, it was a tricky race condition. I like the approach and am about to use it, so I am reviewing it, trying to make sure its solid. Asked both to share for others, but also in case I was missing something about it. Thanks for following up :) – eglasius Jul 05 '21 at 12:55
  • 2
    @xtadex did not find anything else, here is the version I use with slight refactor to my taste and some notes https://github.com/copenhagenatomics/CA_DataUploader/pull/90/files#diff-24a9664c904fe9276878f37dc1438aae578a76b7ef34eabbebf6ac66eaad83e6.. Thanks! – eglasius Jul 13 '21 at 16:19
  • 2
    This version worked great with the Unit Test I created to validate -- while the most recent (and seemingly more simple) version by Alexandar failed this same test with a deadlock. But I really like the `using() {}` notation that `IDisposable` provides so I adapted the version from @eglasius to support that here: https://gist.github.com/cajuncoding/a88f0d00847dcfc241ae80d1c7bafb1e – CajunCoding Mar 09 '23 at 03:14
  • @CajunCoding I like it, thanks for sharing :). I have to ask, are those unit tests in a public repository by any change? – eglasius Mar 09 '23 at 11:47
  • @CajunCoding in my version I am removing the public ReleaseWriter/Reader methods https://github.com/copenhagenatomics/CA_DataUploader/pull/225 (if needed people can always do try/finally with the returned lock object). – eglasius Mar 09 '23 at 12:49
  • @eglasius glad you found it helpful, when I get some time in coming days I’ll post the unit tests to a gist. – CajunCoding Mar 10 '23 at 07:46
  • 1
    @eglasius I've done the same now in my instance (updated the gist) so that only the `using() {}` syntax is used since it makes the code much more concise. And, as requested I added the Unit Test as a followup Comment on my gist here: https://gist.github.com/cajuncoding/a88f0d00847dcfc241ae80d1c7bafb1e?permalink_comment_id=4498792#gistcomment-4498792 – CajunCoding Mar 11 '23 at 01:32
3

https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim?view=net-5.0

From source:

ReaderWriterLockSlim has managed thread affinity; that is, each Thread object must make its own method calls to enter and exit lock modes. No thread can change the mode of another thread.

So here expected behavour. Async / await does not guarantee continuation in the same thread, so you can catch exception when you enter in write lock in one thread and try to exit in other thread.

Better to use other lock mechanisms from other answers like SemaphoreSlim.

Anton Semenov
  • 411
  • 5
  • 11
0

Like Stephen Cleary says, ReaderWriterLockSlim is a thread-affine lock type, so it usually cannot be used with async and await.


You have to build a mechanism to avoid readers and writers accessing shared data at the same time. This algorithm should follow a few rules.

When requesting a readerlock:

  • Is there a writerlock active?
  • Is there anything already queued? (I think it's nice to execute it in order of requests)

When requesting a writerlock:

  • Is there a writerlock active? (because writerlocks shouldn't execute parallel)
  • Are there any reader locks active?
  • Is there anything already queued?

If any of these creteria answers yes, the execution should be queued and executed later. You could use TaskCompletionSources to continue awaits.

When any reader or writer is done, You should evaluate the queue and continue execute items when possible.


For example (nuget): AsyncReaderWriterLock

Jeroen van Langen
  • 21,446
  • 3
  • 42
  • 57