Skip to content

Commit

Permalink
Clean up the Dispose Method
Browse files Browse the repository at this point in the history 
This commit fixes a race condition in Dispose where we could be in the middle
of inserting an object while the Dispose executes
  • Loading branch information
@anaisbetts
anaisbetts committed Feb 22, 2013
1 parent aa3550e commit ce9c8a7
Showing 1 changed file with 12 additions and 27 deletions.
39 changes: 12 additions & 27 deletions Akavache/PersistentBlobCache.cs
View file
Expand Up @@ -262,47 +262,32 @@ public IObservable<Unit> InvalidateAll()

public void Dispose()
{
// We need to make sure that all outstanding writes are flushed
// before we bail
AsyncSubject<byte[]>[] requests;

if (MemoizedRequests == null)
{
return;
}

lock (MemoizedRequests)
{
var mq = Interlocked.Exchange(ref MemoizedRequests, null);
if (mq == null)
{
return;
}
var mq = Interlocked.Exchange(ref MemoizedRequests, null);

This comment has been minimized.

Sign in to view

Copy link
@haacked

haacked Feb 23, 2013

Contributor

Shoot. Fat fingered that last comment. Paul, I thought of another race condition that's probably way more likely than the fanciful ones I came up with earlier.

Since we don't lock on MemoizedRequests here, we have the very real risk that we will set MemoizedRequests to null in the middle of another operation. And that operation could run into a NullReferenceException. For example, InvalidateKey could be within its lock at the same time that Dispose is called.

Here's some code that demonstrates the issue.

public class Tests
{
    object sync = new object();

    [Fact]
    public void Foo()
    {
        var thread1 = new Thread(DoStuffWithLock);
        var thread2 = new Thread(SetStuffNullWithInterlocked);

        thread1.Start();
        Thread.Sleep(100);
        thread2.Start();
        thread1.Join();
        thread2.Join();
    }

    void DoStuffWithLock()
    {
        lock (sync)
        {
            Thread.Sleep(1000);
            Console.WriteLine(sync == null ? "Whoops! NullRef!" : "It's fine guise.");
        }
    }

    void SetStuffNullWithInterlocked()
    {
        var s = Interlocked.Exchange(ref sync, null);
        if (s == null) return;
        Console.WriteLine("Set sync to null");
    }
}
disposed = true;

requests = mq.CachedValues().ToArray();
if (mq == null) return;

MemoizedRequests = null;
// We need to make sure that all outstanding writes are flushed
// before we bail
var requests = mq.CachedValues().ToArray();

actionTaken.OnCompleted();
flushThreadSubscription.Dispose();
}
actionTaken.OnCompleted();
flushThreadSubscription.Dispose();

IObservable<byte[]> requestChain = null;
var requestChain = Observable.Return(new byte[0]);

if (requests.Length > 0)
{
// Since these are all AsyncSubjects, most of them will replay
// immediately, except for the ones still outstanding; we'll
// Merge them all then wait for them all to complete.
requestChain = requests.Merge(System.Reactive.Concurrency.Scheduler.Immediate)
.Timeout(TimeSpan.FromSeconds(30), Scheduler)
.Aggregate((acc, x) => x);
.Timeout(TimeSpan.FromSeconds(30), Scheduler)
.Aggregate((acc, x) => x);
}

requestChain = requestChain ?? Observable.Return(new byte[0]);

// NB: FlushCacheIndex is guaranteed not to throw
requestChain.SelectMany(FlushCacheIndex(true)).Subscribe(_ => {});
disposed = true;
}

/// <summary>
Expand Down

0 comments on commit ce9c8a7

Please sign in to comment.