Simple asynchronous operation implementation

Could you please see if they are no bugs in this implementation? I am especially interested in knowing if there could be here any multithreading problems (such as race conditions).

It is written in C# for .NET 3.5.

```
public interface IAsyncOperation
{
OperationToken Token { get; }
bool IsCompleted { get; }
Exception Exception { get; }
bool Wait(int timeout);
}

public interface IAsyncOperation : IAsyncOperation
{
TResult Result { get; }
}

///
/// Asynchronous operation that may be executed on a separate thread.
/// IsCompleted and Exception would be synchronized after invoking Wait.
///
public abstract class AsyncOperation : IAsyncOperation
{
protected readonly object CompletionLock = new object();
private volatile bool isCompleted; // volatile in order not to change the order of writing Exception and Result

protected AsyncOperation(bool isCompleted = false)
{
IsCompleted = isCompleted;
Token = OperationToken.New();
}

public OperationToken Token { get; protected set; }

public bool IsCompleted
{
get { return isCompleted; }
protected set { isCompleted = value; }
}

public Exception Exception { get; protected set; }

public bool Wait(int timeout)
{
if (!IsCompleted)
{
lock (CompletionLock)
{
if (!IsCompleted)
{
// when migrated to .NET 4.5 then implement with ManualResetEventSlim
Monitor.Wait(CompletionLock, timeout);
}
}
}

return IsCompleted;
}

protected void Complete()
{
if (IsCompleted)
{
throw new InvalidOperationException("The operation was already completed");
}

lock (CompletionLock)
{
if (IsCompleted)
{
throw new InvalidOperationException("The operation was already completed");
}

-
For this:

protected void Complete()
{
    if (!IsCompleted)
    {
        lock (CompletionLock)
        {
            if (!IsCompleted)
            {
                IsCompleted = true;
                Monitor.PulseAll(CompletionLock);
            }
        }
    }
}

I guess if a client/subclass calls Complete() twice it's an error in the client code. You may want to fail early and throw an exception. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

Inverting the first condition and using a guard clause also would make the code more flatten:

protected void Complete()
{
    if (IsCompleted)
    {
        throw new ...;
    }
    lock (CompletionLock)
    {
        if (IsCompleted)
        {
            throw new ...;
        }
        IsCompleted = true;
        Monitor.PulseAll(CompletionLock);
    }
}

You could extract a CheckNotCompleted method here to remove some duplicateion:

protected void Complete()
{
    CheckNotCompleted();
    lock (CompletionLock)
    {
        CheckNotCompleted();
        IsCompleted = true;
        Monitor.PulseAll(CompletionLock);
    }
}

private void CheckNotCompleted() 
{
    if (IsCompleted)
    {
        throw new ...;
    }
}

-
I'd consider using composition instead of inheritance. Effective Java, Second Edition, Item 16: Favor composition over inheritance is a good resource about that.

-
The code reminds me Futures and Executors from Java.

In Java, you can create an ExecutorService with Executors.newFixedThreadPool() (and with many other methods).

int threadNumber = 1;
ExecutorService executorService = Executors.newFixedThreadPool(threadNumber);

Then you can submit a task instance which implements Runnable or Callable which will be run on another thread and you get a Future instance immediately:

Future future = executorService.submit(task);

Future has isDone() and get(timeout, timeUnit) methods which are very similar to your IsCompleted and Wait methods.

I feel that I've lost a little bit in the C# classes (I'm not too familiar with C#) but I hope the design of Future could give you some ideas.

StackExchange Code Review Q#44326, answer score: 5