Fix deadlock bug in ipcache (#43)

* Fix deadlock bug in ipcache
* Make sure autotools are installed for build

Author: pboothe

Dockerfile

@@ -11,9 +11,9 @@ RUN chmod -R a+rx /go/bin/traceroute-caller
 
 
 FROM ubuntu:latest
-# Install all the standard packages we need and then remove the ap-get lists.
+# Install all the standard packages we need and then remove the apt-get lists.
 RUN apt-get update && \
-    apt-get install -y python python-pip make iproute2 coreutils && \
+    apt-get install -y python python-pip make iproute2 coreutils autoconf && \
     apt-get clean && \
     rm -rf /var/lib/apt/lists/*
 

ipcache/ipcache.go

@@ -28,38 +28,45 @@ type Tracer interface {
 type cachedTest struct {
 	timeStamp time.Time
 	data      string
-	done      chan struct{}
+	dataReady chan struct{}
 }
 
 // RecentIPCache contains a list of all the IP addresses that we have traced to
 // recently. We keep this list to ensure that we don't traceroute to the same
 // location repeatedly at a high frequency.
 type RecentIPCache struct {
 	cache map[string]*cachedTest
-	mu    sync.RWMutex
+	mu    sync.Mutex
 
 	tracer Tracer
 }
 
-// Trace performs a trace and adds it to the cache.
-func (rc *RecentIPCache) Trace(conn connection.Connection) {
-	ip := conn.RemoteIP
+func (rc *RecentIPCache) getEntry(ip string) (*cachedTest, bool) {
 	rc.mu.Lock()
-	_, ok := rc.cache[ip]
-	if !ok {
-		nc := &cachedTest{
+	defer rc.mu.Unlock()
+	_, existed := rc.cache[ip]
+	if !existed {
+		rc.cache[ip] = &cachedTest{
 			timeStamp: time.Now(),
-			done:      make(chan struct{}),
+			dataReady: make(chan struct{}),
 		}
-		rc.cache[ip] = nc
-		rc.mu.Unlock()
+	}
+	return rc.cache[ip], existed
+}
 
-		nc.data = rc.tracer.Trace(conn, nc.timeStamp)
-		close(nc.done)
-		return
+// Trace performs a trace and adds it to the cache. It calls the methods of the
+// tracer, so if those create files on disk, then files on disk will be created
+// as a side effect.
+func (rc *RecentIPCache) Trace(conn connection.Connection) string {
+	c, cached := rc.getEntry(conn.RemoteIP)
+	if cached {
+		<-c.dataReady
+		rc.tracer.CreateCacheTest(conn, time.Now(), c.data)
+		return c.data
 	}
-	rc.mu.Unlock()
-	rc.tracer.CreateCacheTest(conn, time.Now(), rc.GetData(ip))
+	c.data = rc.tracer.Trace(conn, c.timeStamp)
+	close(c.dataReady)
+	return c.data
 }
 
 // GetCacheLength returns the number of items currently in the cache. The
@@ -69,19 +76,6 @@ func (rc *RecentIPCache) GetCacheLength() int {
 	return len(rc.cache)
 }
 
-// GetData will wait till the test content available if there is an entry
-// in cache.
-func (rc *RecentIPCache) GetData(ip string) string {
-	rc.mu.RLock()
-	defer rc.mu.RUnlock()
-	c, ok := rc.cache[ip]
-	if ok {
-		<-c.done
-		return c.data
-	}
-	return ""
-}
-
 // New creates and returns a RecentIPCache. It also starts up a background
 // goroutine that scrubs the cache.
 func New(ctx context.Context, tracer Tracer, ipCacheTimeout, ipCacheUpdatePeriod time.Duration) *RecentIPCache {
@@ -96,13 +90,14 @@ func New(ctx context.Context, tracer Tracer, ipCacheTimeout, ipCacheUpdatePeriod
 			if ctx.Err() != nil {
 				return
 			}
+			// Must hold lock while performing GC.
+			m.mu.Lock()
 			for k, v := range m.cache {
 				if now.Sub(v.timeStamp) > ipCacheTimeout {
-					m.mu.Lock()
 					delete(m.cache, k)
-					m.mu.Unlock()
 				}
 			}
+			m.mu.Unlock()
 		}
 
 	}()

ipcache/ipcache_test.go

@@ -2,7 +2,6 @@ package ipcache_test
 
 import (
 	"context"
-	"log"
 	"testing"
 	"time"
 
@@ -12,45 +11,19 @@ import (
 
 type testTracer struct {
 	calls   int
+	cctest  int
 	answers []map[connection.Connection]struct{}
 }
 
 func (tf *testTracer) Trace(conn connection.Connection, t time.Time) string {
-	log.Println("Create Trace Test")
-	return "Fake Trace test " + conn.RemoteIP
+	return "Fake trace test " + conn.RemoteIP
 }
 
 func (tf *testTracer) CreateCacheTest(conn connection.Connection, t time.Time, cachedTest string) {
-	log.Println("Create cached Test " + conn.RemoteIP)
+	tf.cctest++
 	return
 }
 
-func TestGetData(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-	var tt testTracer
-	testCache := ipcache.New(ctx, &tt, 100*time.Second, time.Second)
-
-	conn1 := connection.Connection{
-		RemoteIP:   "1.1.1.2",
-		RemotePort: 5034,
-		LocalIP:    "1.1.1.3",
-		LocalPort:  58790,
-		Cookie:     "10f3d"}
-
-	testCache.Trace(conn1)
-	time.Sleep(200 * time.Millisecond)
-	tmp := testCache.GetData("1.1.1.1")
-	if tmp != "" {
-		t.Error("GetData not working correctly ")
-	}
-
-	tmp = testCache.GetData("1.1.1.2")
-	if tmp != "Fake Trace test 1.1.1.2" {
-		t.Error("GetData not working correctly ")
-	}
-}
-
 func TestTrace(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -64,25 +37,29 @@ func TestTrace(t *testing.T) {
 		LocalPort:  58790,
 		Cookie:     "10f3d"}
 
-	testCache.Trace(conn1)
-	time.Sleep(200 * time.Millisecond)
-	tmp := testCache.GetData("1.1.1.2")
-	if tmp != "Fake Trace test 1.1.1.2" {
+	tmp := testCache.Trace(conn1)
+	if tmp != "Fake trace test 1.1.1.2" {
 		t.Error("cache not trace correctly ")
 	}
 
 	conn2 := connection.Connection{
 		RemoteIP:   "1.1.1.5",
 		RemotePort: 5034,
-		LocalIP:    "1.1.1.7",
+		LocalIP:    "1.1.1.3",
 		LocalPort:  58790,
 		Cookie:     "aaaa"}
 
-	testCache.Trace(conn2)
-	testCache.Trace(conn1)
-
-	time.Sleep(200 * time.Millisecond)
-
+	t2 := testCache.Trace(conn2)
+	if t2 != "Fake trace test 1.1.1.5" {
+		t.Error("cache did not trace")
+	}
+	if tt.cctest != 0 {
+		t.Errorf("Should have had zero calls to CreateCachedTest, not %d", tt.cctest)
+	}
+	testCache.Trace(conn1) // This should be cached
+	if tt.cctest != 1 {
+		t.Errorf("Should have had one call to CreateCachedTest, not %d", tt.cctest)
+	}
 	if testCache.GetCacheLength() != 2 {
 		t.Error("cache not working correctly ")
 	}
@@ -101,12 +78,12 @@ func TestRecentIPCache(t *testing.T) {
 		Cookie:     "11",
 	})
 	if tmp.GetCacheLength() != 1 {
-		t.Error("cache not working correctly")
+		t.Error("Did not put an entry into the cache")
 	}
 
 	time.Sleep(300 * time.Millisecond)
 	if tmp.GetCacheLength() != 0 {
-		t.Error("cache not working correctly")
+		t.Error("Cache GC failed to collect garbage")
 	}
 	cancel()
 	time.Sleep(200 * time.Millisecond)

scamper/scamper.go

@@ -227,4 +227,3 @@ func (d *Daemon) trace(conn connection.Connection, t time.Time) string {
 	}
 	return string(buff.Bytes())
 }
-