Merge lp:~dave-cheney/goose/tip into lp:goose

[Dave Cheney]

testing: fix data race in swift mock

https://codereview.appspot.com/108380043/

testing: fix data race in swift mock

https://codereview.appspot.com/108380043/

[Dave Cheney (dave-cheney)]

Reviewers: mp+225257_code.launchpad.net,

Message:
Please take a look.

Description:
testing: fix data race in swift mock

https://code.launchpad.net/~dave-cheney/goose/tip/+merge/225257

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/108380043/

Affected files (+19, -0 lines):
   A [revision details]
   M testservices/swiftservice/service.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140613065325-tlt6r3jg7saby4ub
+New revision: <email address hidden>

Index: testservices/swiftservice/service.go
=== modified file 'testservices/swiftservice/service.go'
--- testservices/swiftservice/service.go 2013-09-04 09:50:54 +0000
+++ testservices/swiftservice/service.go 2014-07-02 05:34:48 +0000
@@ -10,6 +10,7 @@
   "net/url"
   "sort"
   "strings"
+ "sync"
   "time"
  )

@@ -20,6 +21,8 @@

  type Swift struct {
   testservices.ServiceInstance
+
+ mu sync.Mutex // protects the remaining fields
   containers map[string]object
  }

@@ -70,7 +73,9 @@

  // HasContainer verifies the given container exists or not.
  func (s *Swift) HasContainer(name string) bool {
+ s.mu.Lock()
   _, ok := s.containers[name]
+ s.mu.Unlock()
   return ok
  }

@@ -80,7 +85,9 @@
   if err := s.ProcessFunctionHook(s, container, name); err != nil {
    return nil, err
   }
+ s.mu.Lock()
   data, ok := s.containers[container][name]
+ s.mu.Unlock()
   if !ok {
    return nil, fmt.Errorf("no such object %q in container %q", name,
container)
   }
@@ -96,7 +103,9 @@
   if s.HasContainer(name) {
    return fmt.Errorf("container already exists %q", name)
   }
+ s.mu.Lock()
   s.containers[name] = make(object)
+ s.mu.Unlock()
   return nil
  }

@@ -110,7 +119,9 @@
   if ok := s.HasContainer(name); !ok {
    return nil, fmt.Errorf("no such container %q", name)
   }
+ s.mu.Lock()
   items := s.containers[name]
+ s.mu.Unlock()
   sorted := make([]string, 0, len(items))
   prefix := params["prefix"]
   for filename := range items {
@@ -154,7 +165,9 @@
     return err
    }
   }
+ s.mu.Lock()
   s.containers[container][name] = data
+ s.mu.Unlock()
   return nil
  }

@@ -166,7 +179,9 @@
   if ok := s.HasContainer(name); !ok {
    return fmt.Errorf("no such container %q", name)
   }
+ s.mu.Lock()
   delete(s.containers, name)
+ s.mu.Unlock()
   return nil
  }

@@ -178,7 +193,9 @@
   if _, err := s.GetObject(container, name); err != nil {
    return err
   }
+ s.mu.Lock()
   delete(s.containers[container], name)
+ s.mu.Unlock()
   return nil
  }

[Andrew Wilkins (axwalk)]

On 2014/07/02 05:36:39, dfc wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/108380043/

=== modified file 'testservices/swiftservice/service.go'
--- testservices/swiftservice/service.go	2013-09-04 09:50:54 +0000
+++ testservices/swiftservice/service.go	2014-07-02 05:35:39 +0000
@@ -10,6 +10,7 @@
 	"net/url"
 	"sort"
 	"strings"
+	"sync"
 	"time"
 )
 
@@ -20,6 +21,8 @@
 
 type Swift struct {
 	testservices.ServiceInstance
+
+	mu         sync.Mutex // protects the remaining fields
 	containers map[string]object
 }
 
@@ -70,7 +73,9 @@
 
 // HasContainer verifies the given container exists or not.
 func (s *Swift) HasContainer(name string) bool {
+	s.mu.Lock()
 	_, ok := s.containers[name]
+	s.mu.Unlock()
 	return ok
 }
 
@@ -80,7 +85,9 @@
 	if err := s.ProcessFunctionHook(s, container, name); err != nil {
 		return nil, err
 	}
+	s.mu.Lock()
 	data, ok := s.containers[container][name]
+	s.mu.Unlock()
 	if !ok {
 		return nil, fmt.Errorf("no such object %q in container %q", name, container)
 	}
@@ -96,7 +103,9 @@
 	if s.HasContainer(name) {
 		return fmt.Errorf("container already exists %q", name)
 	}
+	s.mu.Lock()
 	s.containers[name] = make(object)
+	s.mu.Unlock()
 	return nil
 }
 
@@ -110,7 +119,9 @@
 	if ok := s.HasContainer(name); !ok {
 		return nil, fmt.Errorf("no such container %q", name)
 	}
+	s.mu.Lock()
 	items := s.containers[name]
+	s.mu.Unlock()
 	sorted := make([]string, 0, len(items))
 	prefix := params["prefix"]
 	for filename := range items {
@@ -154,7 +165,9 @@
 			return err
 		}
 	}
+	s.mu.Lock()
 	s.containers[container][name] = data
+	s.mu.Unlock()
 	return nil
 }
 
@@ -166,7 +179,9 @@
 	if ok := s.HasContainer(name); !ok {
 		return fmt.Errorf("no such container %q", name)
 	}
+	s.mu.Lock()
 	delete(s.containers, name)
+	s.mu.Unlock()
 	return nil
 }
 
@@ -178,7 +193,9 @@
 	if _, err := s.GetObject(container, name); err != nil {
 		return err
 	}
+	s.mu.Lock()
 	delete(s.containers[container], name)
+	s.mu.Unlock()
 	return nil
 }