5

Using what I judged was the best of all worlds on the Implementing the Singleton Pattern in C# amazing article, I have been using with success the following class to persist user-defined data in memory (for the very rarely modified data):

public class Params
{
  static readonly Params Instance = new Params();

  Params()
  {
  }

  public static Params InMemory
  {
    get
    {
      return Instance;
    }
  }

  private IEnumerable<Localization> _localizations;

  public IEnumerable<Localization> Localizations
  {
    get
    {
      return _localizations ?? (_localizations = new Repository<Localization>().Get());
    }
  }

  public int ChunkSize
  {
    get
    {
      // Loc uses the Localizations impl
      LC.Loc("params.chunksize").To<int>();
    }
  }

  public void RebuildLocalizations()
  {
    _localizations = null;
  }

  // other similar values coming from the DB and staying in-memory,
  // and their refresh methods

}

My usage would look something like this:

var allLocs = Params.InMemory.Localizations; //etc

Whenever I update the database, the RefreshLocalizations gets called, so only part of my in-memory store is rebuilt. I have a single production environment out of about 10 that seems to be misbehaving when the RefreshLocalizations gets called, not refreshing at all, but this is also seems to be intermittent and very odd altogether.

My current suspicions goes towards the singleton, which I think does the job great and all the unit tests prove that the singleton mechanism, the refresh mechanism and the RAM performance all work as expected.

That said, I am down to these possibilities:

  1. This customer is lying when he says their environment is not using loading balance, which is a setting I am not expecting the in-memory stuff to work properly (right?)
  2. There is some non-standard pool configuration in their IIS which I am testing against (maybe in a Web Garden setting?)
  3. The singleton is failing somehow, but not sure how.

Any suggestions?

.NET 3.5 so not much parallel juice available, and not ready to use the Reactive Extensions for now

Edit1: as per the suggestions, would the getter look something like:

public IEnumerable<Localization> Localizations
{
  get
  {
    lock(_localizations) {
      return _localizations ?? (_localizations = new Repository<Localization>().Get());
    }
  }
}
Improve this question
12
  • 3
    Is is possible you have some thread safety issues here? Seems like you would want to have a lock to prevent multiple instances of your repository from being created after a refresh (commonly called a "cache stampede") Commented Jan 11, 2012 at 15:15
  • 1
    From you question it sounds like the code to post would be the code that does the refresh. What you have above looks ok to me. Load balancing and IIS should not have any effect (but again would need to see the db refresh code), unless you are referring to multiple webservers in which case you will need to synchronise the refresh across them (which which case just use an external cache....AppFabric cache would be suitable) Commented Jan 11, 2012 at 15:20
  • 1
    @F.Aquino: Actually, no, it doesn't. It could very well be that two threads try to access the Localization property at nearly the same time, leading to two repositories being created. Making this property thread safe will get rid of this problem. In other words: Your singleton class is not thread safe after the instance has been created. Commented Jan 11, 2012 at 15:20
  • 1
    As Eric said, there definitely could be some threading issues as you are not locking around the _localizations member (and the ?? call). However you said the data is rarely updated...are you sure of that? Commented Jan 11, 2012 at 15:22
  • 2
    @F.Aquino I should add that you should amend your Singleton to account for the threading issues mentioned by the other posters, but I don't think thats the source of your problem. Also, is there any reason you didn't use a simple static class? Singletons can be useful if you are implmenting an interface, or need to inherit from a base class, but you don't seem to be doing that, so a static class might be just as effective. Commented Jan 11, 2012 at 16:03

2 Answers 2

Reset to default
2

To expand on my comment, here is how you might make the Localizations property thread safe:

public class Params
{
  private object _lock = new object();

  private IEnumerable<Localization> _localizations;    
  public IEnumerable<Localization> Localizations
  {
    get
    {
      lock (_lock) {
         if ( _localizations == null ) {
            _localizations = new Repository<Localization>().Get();
         }

         return _localizations;
      }
    }
  }

  public void RebuildLocalizations()
  {
     lock(_lock) {
        _localizations = null;
     }
  }

  // other similar values coming from the DB and staying in-memory,
  // and their refresh methods

}
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Eric, thanks, the _lock approach has any advantages from locking the instance variable itself?
@F.Aquino - Yes, since you are assigning to the instance variable, you don't also want to lock on it as your lock will be inadvertently released when you reassign the object. See the first answer on this question for a good explaination: stackoverflow.com/questions/1287356/…
Thank you Erik, I went with your suggesting and will start the QA today, I would like to thank everyone else that contributed to the thread, amazing and quick feedback, sadly these major questions that I see so many people getting wrong aren't asked as much.
2

There is no point in creating a thread safe singleton, if your properties are not going to be thread safe.

You should either lock around assignment of the _localization field, or instantiate in your singleton's constructor (preferred). Any suggestion which applies to singleton instantiation applies to this lazy-instantiated property.

The same thing further applies to all properties (and their properties) of Localization. If this is a Singleton, it means that any thread can access it any time, and simply locking the getter will again do nothing.

For example, consider this case:

    Thread 1                              Thread 2

    // both threads access the singleton, but you are "safe" because you locked
1.  var loc1 = Params.Localizations;      var loc2 = Params.Localizations;

    // do stuff                           // thread 2 calls the same property...
2.  var value = loc1.ChunkSize;           var chunk = LC.Loc("params.chunksize");

    // invalidate                         // ...there is a slight pause here...
3.  loc1.RebuildLocalizations();

                                          // ...and gets the wrong value
4.                                        var value = chunk.To();

If you are only reading these values, then it might not matter, but you can see how you can easily get in trouble with this approach.

Remember that with threading, you never know if a different thread will execute something between two instructions. Only simple 32-bit assignments are atomic, nothing else.

This means that, in this line here:

return LC.Loc("params.chunksize").To<int>();

is, as far as threading is concerned, equivalent to:

var loc = LC.Loc("params.chunksize");
Thread.Sleep(1); // anything can happen here :-(
return loc.To<int>();

Any thread can jump in between Loc and To.

Improve this answer

4 Comments

Your second approach is something I've never seen, very interesting, so the constructor approach won't require the lock wrapper? Would that also eliminate the instance variable _localizations and I would simply set the Localizations property using an auto-property?
@F.Aquino: yes and no; it would still imply a private readonly backing field _localizations, to prevent anyone from accidentally changing it, since there is no locking. Leaving the set accessor would fix nothing. But in that case, you wouldn't have the possibility to "rebuild" it by invalidating it. If this is what you are after, go for regular locking, that's your only option.
@F.Aquino: I've updated my answer with additional info. Btw, why is this called "Localizations"? What does it have with localization actually?
It is a quick translation from the actual portuguese domain-specific name for a multi-language site, probably poorly chosen.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.