views:

126

answers:

4

I have class Lazy which lazily evaluates an expression:

public sealed class Lazy<T>
{
    Func<T> getValue;
    T value;

    public Lazy(Func<T> f)
    {
        getValue = () =>
            {
                lock (getValue)
                {
                    value = f();
                    getValue = () => value;
                }
                return value;
            };
    }

    public T Force()
    {
        return getValue();
    }
}

Basically, I'm trying to avoid the overhead of locking objects after they've been evaluated, so I replace getValue with another function on invocation.

It apparently works in my testing, but I have no way of knowing if it'll blow up in production.

Is my class threadsafe? If not, what can be done to guarantee thread safety?

A: 

I'm not entirely sure what you're trying to do with this code, but I just published an article on The Code Project on building a sort of "lazy" class that automatically, asynchronously calls a worker function and stores its value.

Adam Maras
Very interesting article :) But its not "lazy" in the sense that most people are interested in. Two use cases stand out: 1) most ORMs use lazy loading to prevent instantiating a databases entire object graph in memory at once. 2) A whole class of immutable data structures (notable streams and some implementations of queues) depends on laziness for certain amortized performance characteristics.
Juliet
+1  A: 

Can’t you just omit re-evaluating the function completely by either using a flag or a guard value for the real value? I.e.:

public sealed class Lazy<T>
{
    Func<T> f;
    T value;
    volatile bool computed = false;
    void GetValue() { lock(LockObject) { value = f();  computed = true; } }

    public Lazy(Func<T> f)
    {
        this.f = f;
    }

    public T Force()
    {
        if (!computed) GetValue();
        return value;
    }
}
Konrad Rudolph
Your code is not thread-safe, the optimizer may reorder the writes or reads in your code.To make your code safe, you'll have to mark "bool computed" as volatile.In general, whenever you read a variable without locking that might be set from another thread, you need to think about the memory model and where you need barriers or volatile reads/writes.
Daniel
+1, +answer: I have a bad tendency to wear complicator's gloves, and that's precisely the reason I should never be allowed to touch code :) As noted above, I think marking a flag as volatile will get the results I want with much less "magic".
Juliet
@Daniel: why would the variable "computed" need to be marked volatile if its already contained in the lock statement? Would the variable "value" need to be marked volatile for the same reasons?
Juliet
"computed" needs to be marked volatile because it is accessed without proper locking: there is no lock when reading the variable.Marking "computed" as volatile guarantees that the write to it is executed after preceding writes issued by that thread, and that it is read before other reads are issued by the reading thread. See http://www.albahari.com/threading/part4.aspx#_NonBlockingSynch for details."value" does not need volatile because it is read after "computed" was already observed to be true.
Daniel
A: 

This looks more like a caching mechanism than a "lazy evaluation". In addition, do not change the value of a locking reference within the lock block. Use a temporary variable to lock on.

The wait you have it right now would work in a large number of cases, but if you were to have two different threads try to evaluate the expression in this order:

Thread 1
Thread 2
Thread 1 completes

Thread 2 would never complete, because Thread 1 would be releasing a lock on a different reference than was used to acquire the lock (more precisely, he'd be releasing a nonexistent lock, since the newly-created reference was never locked to begin with), and not releasing the original lock, which is blocking Thread 2.

While I'm not entirely certain what this would do (aside from perform a synchronized evaluation of the expression and caching of the result), this should make it safer:

public sealed class Lazy<T>
{
    Func<T> getValue;
    T value;
    object lockValue = new object();

    public Lazy(Func<T> f)
    {
        getValue = () =>
            {
                lock (lockValue)
                {
                    value = f();
                    getValue = () => value;
                }
                return value;
            };
    }

    public T Force()
    {
        return getValue();
    }
}
Adam Robinson
This now ensures the f() isn't called concurrently; but it can still be called multiple times.
Daniel
@Daniel: Yes, you're right. The lock would need to be moved to the `Force` method in order to prevent that.
Adam Robinson
+1  A: 

Your code has a few issues:

  1. You need one object to do the locking on. Don't lock on a variable that gets changed - locks always deal with objects, so if getValue is changed, multiple threads might enter the locked section at once.

  2. If multiple threads are waiting for the lock, all of them will evaluate the function f() after each other. You'd have to check inside the lock that the function wasn't evaluated already.

  3. You might need a memory barrier even after fixing the above issues to ensure that the delegate gets replaced only after the new value was stored to memory.

However, I'd use the flag approach from Konrad Rudolph instead (just ensure you don't forget the "volatile" required for that). That way you don't need to invoke a delegate whenever the value is retrieved (delegate calls are quite fast; but not they're not as fast as simply checking a bool).

Daniel