ReaderWriterLock not working in ServiceBehavior constructor

Question

I have a WCF service where InstanceContextMode is Single and ConcurrencyMode is Multiple. The aim is to create a cache of values on instantiation, without holding up other service calls not reliant upon the creation of the cache.

This way only methods that attempt to obtain a read lock on _classificationsCacheLock will need to wait until the value of classificationsCache is populated (classificationsCacheLock.IsWriterLockHeld = false).

However the issue is, despite acquiring a write Lock in the task thread, the call WCF continues to service in response to a call into the service method GetFOIRequestClassificationsList() results in _classificationsCacheLock.IsWriterLockHeld being false, when it should be true.

Is this odd behavior with WCF instancing or is this me fundamentally missing a trick.

I tried both acquiring the write lock within the thread context of the constructor (safe option) and within the context of the spawned task thread (which could introduce a race between WCF then invoking the call to the GetFOIRequestClassificationsList() function faster than call to classificationsCacheLock.AcquireWriterLock(Timeout.Infinite);) but both result in classificationsCacheLock.IsWriterLockHeld being false despite having prevented any race condition via the use of thread.sleep, staggered appropriately apart in the code blocks of each respective thread.

[ServiceBehavior(Namespace = Namespaces.MyNamespace,
    ConcurrencyMode = ConcurrencyMode.Multiple,
InstanceContextMode = InstanceContextMode.Single)]
[AspNetCompatibilityRequirements(RequirementsMode = AspNetCompatibilityRequirementsMode.Allowed)]
public class MyService : IMyService
{
    private static NLog.Logger _logger = NLog.LogManager.GetCurrentClassLogger();

    private List<string> _classificationsCache;

    private ReaderWriterLock _classificationsCacheLock;

    public MyService()
    {
        try
        {
            _classificationsCacheLock = new ReaderWriterLock();
            LoadCache();
        }
        catch (Exception ex)
        {
            _logger.Error(ex);
        }

    }

    private void LoadCache()
    {
        // _classificationsCacheLock.AcquireWriterLock(Timeout.Infinite); 

        Task.Factory.StartNew(() =>
        {
            try
            {
                _classificationsCacheLock.AcquireWriterLock(Timeout.Infinite); // can only set writer on or off on same thread, not between threads
                if (_classificationsCache == null)
                {
                    var cases = SomeServices.GetAllFOIRequests();
                    _classificationsCache = cases.SelectMany(c => c.Classifications.Classification.Select(cl => cl.Group)).Distinct().ToList();
                }

            }
            catch (Exception ex)
            {

                _logger.Error(ex);

            }
            finally
            {

                if (_classificationsCacheLock.IsWriterLockHeld)
                    _classificationsCacheLock.ReleaseWriterLock();
            }

        });//.ContinueWith((prevTask) =>
        //{
        //     if (_classificationsCacheLock.IsWriterLockHeld)
        //          _classificationsCacheLock.ReleaseWriterLock();
        //  });

    }

     public GetFOIRequestClassificationsList_Response GetFOIRequestClassificationsList()
    {
        try
        {

            GetFOIRequestClassificationsList_Response response = new GetFOIRequestClassificationsList_Response();

            _classificationsCacheLock.AcquireReaderLock(Timeout.Infinite);
            response.Classifications = _classificationsCache;
            _classificationsCacheLock.ReleaseReaderLock();

            return response;

        }
        catch (Exception ex)
        {
            _logger.Error(ex);

            if (ex is FaultException)
            {
                throw;
            }
            else
                throw new FaultException(ex.Message);
        }
    }
}

EDIT 1

Since a number of suggestions were around uncertainty within the thread pool and how a Task might handle thread affinity, I changed the method to explicitly spawn a new thread

var newThread = new Thread(new ThreadStart(() =>
        {
            try
            {
                Thread.Sleep(2000);

                Debug.WriteLine(string.Format("LoadCache - _classificationsCacheLock.GetHashCode - {0}", _classificationsCacheLock.GetHashCode()));
                Debug.WriteLine(string.Format("LoadCache - Thread.CurrentThread.ManagedThreadId-  {0} ", Thread.CurrentThread.ManagedThreadId));


                _classificationsCacheLock.AcquireWriterLock(Timeout.Infinite); // can only set writer on or off on same thread, not between threads
                if (_classificationsCache == null)
                {
                    var cases = SomeServices.GetAllFOIRequests();
                    _classificationsCache = cases.SelectMany(c => c.Classifications.Classification.Select(cl => cl.Group)).Distinct().ToList();
                }

            }
            catch (Exception ex)
            {

                _logger.Error(ex);

            }
            finally
            {

                if (_classificationsCacheLock.IsWriterLockHeld)
                    _classificationsCacheLock.ReleaseWriterLock();
            }

        }));
        newThread.IsBackground = true;
        newThread.Name = "MyNewThread" 
        newThread.Start();

The result is still the same. classificationsCacheLock.AcquireReaderLock does not wait / block as it seemingly should.

I also added some diagnostics to check if;

The thread was in fact the same thread, you cannot expect a R/W to block on the same thread

The instance of the _classificationsCacheLock was identical at all times

public GetFOIRequestClassificationsList_Response GetFOIRequestClassificationsList() { try {

        GetFOIRequestClassificationsList_Response response = new GetFOIRequestClassificationsList_Response();

        Debug.WriteLine(string.Format("GetFOIRequestClassificationsList - _classificationsCacheLock.GetHashCode - {0}", _classificationsCacheLock.GetHashCode()));
        Debug.WriteLine(string.Format("GetFOIRequestClassificationsList - Thread.CurrentThread.ManagedThreadId - {0} ", Thread.CurrentThread.ManagedThreadId));

        Thread.Sleep(1000);

         _classificationsCacheLock.AcquireReaderLock(Timeout.Infinite);
        //_classificationsCacheMRE.WaitOne();

        response.Classifications = _classificationsCache;

        _classificationsCacheLock.ReleaseReaderLock();

        return response;

    }
    catch (Exception ex)
    {
        _logger.Error(ex);

        if (ex is FaultException)
        {
            throw;
        }
        else
            throw new FaultException(ex.Message);
    }
}

The result was ..

GetFOIRequestClassificationsList - _classificationsCacheLock.GetHashCode - 16265870
GetFOIRequestClassificationsList - Thread.CurrentThread.ManagedThreadId - 9 
LoadCache - _classificationsCacheLock.GetHashCode - 16265870
LoadCache - Thread.CurrentThread.ManagedThreadId-  10

.. in that order so now we have a race condition as expected since the write lock was obtained in the newly created thread. The actual WCF Service call is being made before the constructor's spawned thread has been scheduled to actually run. So I move

  _classificationsCacheLock.AcquireWriterLock(Timeout.Infinite);

to the constructor since this is guaranteed to be executed before any class fields are accessed.

Still the AcquireWriterLock does not block, despite the evidence that the constructor was initialized on a different WCF thread to the WCF thread that executes the service method.

 private void LoadCache()
    {

        _classificationsCacheLock.AcquireWriterLock(Timeout.Infinite);

        Debug.WriteLine(string.Format("LoadCache constructor thread - _classificationsCacheLock.GetHashCode - {0}", _classificationsCacheLock.GetHashCode()));
        Debug.WriteLine(string.Format("LoadCache constructor thread - Thread.CurrentThread.ManagedThreadId-  {0} ", Thread.CurrentThread.ManagedThreadId));

        var newThread = new Thread(new ThreadStart(() =>
        {
            try
            {
                Thread.Sleep(5000);

                Debug.WriteLine(string.Format("LoadCache new thread - _classificationsCacheLock.GetHashCode - {0}", _classificationsCacheLock.GetHashCode()));
                Debug.WriteLine(string.Format("LoadCache new thread - Thread.CurrentThread.ManagedThreadId-  {0} ", Thread.CurrentThread.ManagedThreadId));


              //  _classificationsCacheLock.AcquireWriterLock(Timeout.Infinite); // can only set writer on or off on same thread, not between threads
                if (_classificationsCache == null)
                {
                    var cases = SomeServices.GetAllFOIRequests();
                    _classificationsCache = cases.SelectMany(c => c.Classifications.Classification.Select(cl => cl.Group)).Distinct().ToList();
                }

            }
            catch (Exception ex)
            {

                _logger.Error(ex);

            }
            finally
            {

                if (_classificationsCacheLock.IsWriterLockHeld)
                    _classificationsCacheLock.ReleaseWriterLock();
            }

        }));
        newThread.IsBackground = true;
        newThread.Name = "CheckQueues" + DateTime.Now.Ticks.ToString();
        newThread.Start();
}

Again AcquireWriterLock does not block and allows a null reference classificationsCache to be assigned.

The result was ..

LoadCache constructor thread - _classificationsCacheLock.GetHashCode - 22863715
LoadCache constructor thread - Thread.CurrentThread.ManagedThreadId-  9 
GetFOIRequestClassificationsList - _classificationsCacheLock.GetHashCode - 22863715
GetFOIRequestClassificationsList - Thread.CurrentThread.ManagedThreadId - 8 
LoadCache new thread - _classificationsCacheLock.GetHashCode - 22863715
LoadCache new thread - Thread.CurrentThread.ManagedThreadId-  10

EDIT 2

Created a copy of the solution without source control.

Uploaded here if you want to have hands on with the issue.

Changed to using manual reset event in code for demo purposes and commented out problem code.

Place your breakpoints
run Example.Web in Debug
in the browser navigate to 'http://localhost:11164/GetFOIRequestClassificationsList.htm' and click the button.

MRE works, ReaderWriterLock does not work as expected.

.net 4.0 - C#

Print `_classificationsCacheLock.GetHashCode()`. Also, it is usually best to not rely on WCF instancing. I don't see why it is even part of the framework. Put your shared state in some other class and manage it manually (which is easy). — usr, May 17 '15 at 10:20
Interesting and worrying you saying that, would you have any links to a discussion on such problems. Yes ill get that hash of the instance in a bit and see if I am in fact, looking at some sort of object context management issue. — Microsoft Developer, May 18 '15 at 12:00
WCF instancing works as advertised but why take the hassle of understanding and testing it? That's my point. Making a singleton yourself is trivial. Why involve WCF? — usr, May 18 '15 at 12:05
I tend to decide on a concurrency and instancing strategy based on my usage profile, then code to the chosen strategy as needed in WCF. Sticking with the default WCF implementation is not always the best approach for throughput, memory consumption and process efficiency. Using a Monitor (lock {}) solves this but its not ideal, moreover, this is an issue with ReaderWriterLock or my misunderstanding of how it should be used. — Microsoft Developer, May 18 '15 at 12:42
If I am going to have lots of short calls accessing what could be shared data, better to tell WCF not to bother creating new instances for each request. By moving the service into a custom proxy Singleton, I am still left with WCF creating an instance per thread. Idea being, for just a single line attribute, I get an effective Singleton in WCF single mode, also WCF is not performing redundant operations in my instancing strategy, taking requests, fanning out instances only for me to fan back in again delegating to a custom singleton. Still, somethings not working here. — Microsoft Developer, May 18 '15 at 12:43
Since WCF requests are services by IO threads, I wonder if there is some anomaly specific to IO threads and the ReaderWriterLock — Microsoft Developer, May 18 '15 at 12:51
ReaderWriterLock does not have bugs and it does not matter on what thread it runs. The bug is in your code. PerCall instancing is dirt cheap. Creating a nearly empty object costs next to nothing. Compared to WCF request processing the cost as a percentage rounds to zero. — usr, May 18 '15 at 12:56
Perhaps, I wish I knew what the issue was though. Its certainly bugging me. — Microsoft Developer, May 18 '15 at 13:03
Report back when you have done the GetHashCode test. Bad strategy to just stare at the code. Investigate. — usr, May 18 '15 at 13:04
GetFOIRequestClassificationsList = 16265870 GetFOIRequestClassificationsList = 9 LoadCache = 16265870 LoadCache = 10 This is in response to adding the following calls to both the build cache function and the service function. Debug.WriteLine(string.Format("name of function - {0}", _classificationsCacheLock.GetHashCode())); Debug.WriteLine(string.Format("name of function - {0} ", Thread.CurrentThread.ManagedThreadId)); So we have the same hash code but different ManagedThreadId — Microsoft Developer, May 20 '15 at 16:50
I changed the method to spawn a new thread "var newThread = new Thread(new ThreadStart(() => etc". Still does not block as expected. — Microsoft Developer, May 20 '15 at 16:58
That is very strange. Add Debug.Assert(!_classificationsCacheLock.IsWriterLockHeld) to places where you expect the lock to not be held. Also, replace the semaphore with a monitor-based lock (new object()) for testing purposes. Let's see what happens. — usr, May 20 '15 at 17:42
Its very strange, see my updated question with 'edit' for what I have done. I swapped over to using monitor enter / exit and now VS will not launch in debug mode. Crashes each time I try to run the web service in debug :`( — Microsoft Developer, May 20 '15 at 18:01
Hm, at this point I'd run this on a different machine. Maybe a clean VM with a trial VS version that's guaranteed to be clean. — usr, May 20 '15 at 18:11
I copied and cleaned the solution to a new location on disk, now its debugging without crashing. But I have uploaded a copy (see my 'edit 2' above). It would be good to get to the bottom of what the W/R lock is playing up, yet a manual reset event works as expected. — Microsoft Developer, May 21 '15 at 12:08

VMAtm · Answer 1 · 2015-05-18T13:20:29.667

1

In the CaseWork() method you are creating new ReaderWriterLock each time the method is being called. So the lock being aquired simply goes out to the Garbage COllector, and the new one arises. Therefore no lock is actually being correctly aquired.

Why don't you use static lock for this, with creating it in a static constructor?

Correct me if I wrong, but I can't if you are updating your cache. If this is true, I suggest you to simply use the Lazy<T> class. It's thread-safe, and holds all readers before the value is set. It internally uses the TPL, and is simply to use:

private Lazy<List<string>> _classificationsCache = new Lazy<List<string>>
(() => 
{
    var cases = SomeServices.GetAllFOIRequests();
    return cases.SelectMany(c => c.Classifications.Classification.Select(cl => cl.Group)).Distinct().ToList();
});

You can get the value like this:

response.Classifications = _classificationsCache.Value;

Update from MSDN:

If the current thread already has the writer lock, no reader lock is acquired. Instead, the lock count on the writer lock is incremented. This prevents a thread from blocking on its own writer lock. The result is exactly the same as calling AcquireWriterLock, and an additional call to ReleaseWriterLock is required when releasing the writer lock.

I think that this thing is happen:

Your reader lock aquiring is being fired inside the same thread (Task.StartNew method uses the TaskScheduler.Current property) the writer lock is working, so it doesn't block as it has the same priveleges as the Task do, and got the empty list. So in your circumstances you have to choose another one synchronization primitive.

edited May 18 '15 at 13:20

answered May 15 '15 at 17:56

VMAtm

27,943
17
79
125

I must apologies, I missed renaming the original name of the constructor, I have now corrected this. This mislead you to think I was creating new instances of the lock each time. I create only one instance per class instance which is set to single so WCF should only create a single instance of MyService which should service each request. – Microsoft Developer May 16 '15 at 18:52
Indeed, I could use a thread safe collection and perhaps that is what I will end up doing in this particular case. But how strange it is that the ReaderWriterLock seems to be misbehaving like this in this scenario. I can't yet see what I have done (or not done) which would be causing this. – Microsoft Developer May 18 '15 at 11:58
I think that the `Task` you're starting, doesn't run correctly – VMAtm May 18 '15 at 12:44
classificationsCache does get populated once the task has completed, no errors are raised. Meanwhile requests in the background are not being blocked by the call to classificationsCacheLock.AcquireReaderLock and instead get the empty classificationsCache until the task has ran, then each subsequent request gets the populated classificationsCache. This despite placing the acquire write lock in the calling context (commented out) or within the task itself. – Microsoft Developer May 18 '15 at 13:08
Updated the answer. Try to investigate the `CurrentThread.ManagedThreadId`. If it is the same for both `AcquireReaderLock` and `AcquireWriterLock`, than we've found the problem. – VMAtm May 18 '15 at 13:21
@Terry This problem you ran in to is the exact reason it is recommended to use `Task.Run` instead of the TaskFactory – Scott Chamberlain May 20 '15 at 16:35
To rule out task parallel lib issues I changed the method to spawn a new thread "var newThread = new Thread(new ThreadStart(() => etc". Still does not block as expected. – Microsoft Developer May 20 '15 at 17:00
Did you read my update? The reason that your code isn't blocking that the `EnterWriterLock` and `EnterReaderLock` are in the same thread! Use other synchronization structure, for example, lazy loading! – VMAtm May 20 '15 at 19:40
1

Sorry VMAtm, its not the same thread. Your lazy load is an elegant workaround, possibly the better way to achieve the goal ( or in ii7+ use WCF initialiser ), but that still leaves this issue which is indicating strange behavior in .Net, unless we can get to the bottom of why, this could be a .NET issue. I chose not to mark your suggestion as the answer, since it does not get to the bottom of why the R/W lock is not working despite the write block having been obtained on a different thread. – Microsoft Developer May 21 '15 at 12:49

ReaderWriterLock not working in ServiceBehavior constructor

1 Answers1