Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

fix race condition on ObjectLRU #544

Merged
merged 1 commit into from
Aug 12, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions plumbing/cache/object_lru.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cache

import (
"container/list"
"sync"

"gopkg.in/src-d/go-git.v4/plumbing"
)
Expand All @@ -14,6 +15,7 @@ type ObjectLRU struct {
actualSize FileSize
ll *list.List
cache map[interface{}]*list.Element
mut sync.Mutex
}

// NewObjectLRU creates a new ObjectLRU with the given maximum size. The maximum
Expand All @@ -26,6 +28,9 @@ func NewObjectLRU(maxSize FileSize) *ObjectLRU {
// will be marked as used. Otherwise, it will be inserted. A single object might
// be evicted to make room for the new object.
func (c *ObjectLRU) Put(obj plumbing.EncodedObject) {
c.mut.Lock()
defer c.mut.Unlock()

if c.cache == nil {
c.actualSize = 0
c.cache = make(map[interface{}]*list.Element, 1000)
Expand Down Expand Up @@ -67,6 +72,9 @@ func (c *ObjectLRU) Put(obj plumbing.EncodedObject) {
// Get returns an object by its hash. It marks the object as used. If the object
// is not in the cache, (nil, false) will be returned.
func (c *ObjectLRU) Get(k plumbing.Hash) (plumbing.EncodedObject, bool) {
c.mut.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use a RWLock? as suggested at: https://blog.golang.org/go-maps-in-action

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get mutates the state in c.ll.MoveToFront(ee), that's why I used a Mutex instead of a RWMutex

defer c.mut.Unlock()

ee, ok := c.cache[k]
if !ok {
return nil, false
Expand All @@ -78,6 +86,9 @@ func (c *ObjectLRU) Get(k plumbing.Hash) (plumbing.EncodedObject, bool) {

// Clear the content of this object cache.
func (c *ObjectLRU) Clear() {
c.mut.Lock()
defer c.mut.Unlock()

c.ll = nil
c.cache = nil
c.actualSize = 0
Expand Down
28 changes: 28 additions & 0 deletions plumbing/cache/object_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package cache

import (
"fmt"
"io"
"sync"
"testing"

"gopkg.in/src-d/go-git.v4/plumbing"
Expand Down Expand Up @@ -67,6 +69,32 @@ func (s *ObjectSuite) TestClear(c *C) {
c.Assert(obj, IsNil)
}

func (s *ObjectSuite) TestConcurrentAccess(c *C) {
var wg sync.WaitGroup

for i := 0; i < 1000; i++ {
wg.Add(3)
go func(i int) {
s.c.Put(newObject(fmt.Sprint(i), FileSize(i)))
wg.Done()
}(i)

go func(i int) {
if i%30 == 0 {
s.c.Clear()
}
wg.Done()
}(i)

go func(i int) {
s.c.Get(plumbing.NewHash(fmt.Sprint(i)))
wg.Done()
}(i)
}

wg.Wait()
}

type dummyObject struct {
hash plumbing.Hash
size FileSize
Expand Down