tags:

views:

194

answers:

4

I got this síngleton cache object and it exposes an IEnumerable property which just returns a private IEnumerable variable.

I have a static method on my singleton object that updates this member variable (that exists on the single 'Instance' instance of this cache object).

Let's say some thread is currently iterating over this IEnumerable variable/property while my cache is updating. I made it so the cache is updating on a new local variable and finally setting the exposed private variable to point to this new local variable.

I know i'm just updating a reference, leaving the other (old) object in memory waiting to be picked up by the GC but my problem is - i'm not 100% sure what happens once i set the new reference? Would the other thread suddenly be iterating over the new object or the old one it got passed through the IEnumerable interface? If it had been a normal reference i'd say 'no'. The calling thread would be operating on the old object, but i'm not sure if this is the case for IEnumerable as well?

Here is the class stripped down:

internal sealed class SektionCache : CacheBase
{
    public static readonly SektionCache Instance = new SektionCache();
    private static readonly object lockObject = new object();
    private static bool isUpdating;

    private IEnumerable<Sektion> sektioner;

    static SektionCache()
    {
        UpdateCache();
    }

    public IEnumerable<Sektion> Sektioner
    {
        get { return sektioner; }
    }

    public static void UpdateCache()
    {
    // SNIP - getting data, locking etc.
    Instance.sektioner = newSektioner;
    // SNIP
    }
}
+3  A: 

Since the getter { return sektioner; } is called before the new value is put in the field, the old value is returned. Then, the loop foreach (Sektion s in cache.Sektioner) uses the value that was received when the getter was called, i.e. the old value. That value will be used throughout the foreach loop.

configurator
Yeah that's what i'm think too, as it would in any other case, but this particular case raised a question during the review of my object so i wanted to make sure if i didn't miss something.
Per Hornshøj-Schierbeck
A: 

First of all I can't see object locking, unused lockObject variable makes me sad. IEnumerable is not special. Each thread will have it's own copy of reference to some instance of sektioner object. You can't affect other threads that way. What would happen with old version of data pointed by sektioner field largely depends on calling party.

aku
The locking is under the 'SNIP' parts of my UpdateCache :)
Per Hornshøj-Schierbeck
+1  A: 

The thread which is currently enumerating sektioner will continue to enumerate it even when you update the reference within the singleton. There is nothing special about objects which implement IEnumerable.

You should perhaps add the volatile keyword to the sektioner field as you are not providing read-locking and multiple threads are reading/writing it.

Matt Howells
The instance it's enumerating over doesn't need to be new (and the volatile keyword)? I mean i made it IEnumerable so it couldn't change anything to it, or am i missing something here?
Per Hornshøj-Schierbeck
A: 

I think, if you want a thread safety, you should use this way:

internal sealed class SektionCache : CacheBase
{
    //public static readonly SektionCache Instance = new SektionCache();

    // this template is better ( safer ) than the previous one, for thread-safe singleton patter >>>
    private static SektionCache defaultInstance;
    private static object readonly lockObject = new object();
    public static SektionCach Default {
        get {
            SektionCach result = defaultInstance;
            if ( null == result ) {
                lock( lockObject ) {
                    if ( null == result ) {
                        defaultInstance = result = new SektionCache();
                    }
                }
            }

            return result;
        }
    }
    // <<< this template is better ( safer ) than the previous one

    //private static readonly object lockObject = new object();
    //private static bool isUpdating;
    //private IEnumerable<Sektion> sektioner;

    // this declaration is enough
    private volatile IEnumerable<Sektion> sektioner;

    // no static constructor is required >>>
    //static SektionCache()
    //{
    //    UpdateCache();
    //}
    // <<< no static constructor is required

    // I think, you can use getter and setter for reading & changing a collection
    public IEnumerable<Sektion> Sektioner {
        get {
            IEnumerable<Sektion> result = this.sektioner;
            // i don't know, if you need this functionality >>>
            // if ( null == result ) { result = new Sektion[0]; }
            // <<< i don't know, if you need this functionality
            return result;
        }
        set { this.sektion = value; }
    }

    //public static void UpdateCache()
    //{
    //// SNIP - getting data, locking etc.
    //Instance.sektioner = newSektioner;
    //// SNIP
    //}
}
TcKs