From e9a7725ca4e1fdce33501cb1dfa1a17e9c7e6811 Mon Sep 17 00:00:00 2001 From: vince Date: Thu, 13 Aug 2020 17:08:56 +0800 Subject: Add mutex to bugCache This adds a mutex to the bugCache to deal with locking. --- cache/bug_cache.go | 33 +++++++++++++++++++++++++++++++++ cache/repo_cache_test.go | 18 ++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/cache/bug_cache.go b/cache/bug_cache.go index 41e8191d..01d93593 100644 --- a/cache/bug_cache.go +++ b/cache/bug_cache.go @@ -2,6 +2,7 @@ package cache import ( "fmt" + "sync" "time" "github.com/MichaelMure/git-bug/bug" @@ -15,8 +16,10 @@ var ErrNoMatchingOp = fmt.Errorf("no matching operation found") // // 1. Provide a higher level API to use than the raw API from Bug. // 2. Maintain an up to date Snapshot available. +// 3. Deal with concurrency. type BugCache struct { repoCache *RepoCache + mu *sync.RWMutex bug *bug.WithSnapshot } @@ -28,10 +31,14 @@ func NewBugCache(repoCache *RepoCache, b *bug.Bug) *BugCache { } func (c *BugCache) Snapshot() *bug.Snapshot { + c.mu.RLock() + defer c.mu.RUnlock() return c.bug.Snapshot() } func (c *BugCache) Id() entity.Id { + c.mu.RLock() + defer c.mu.RUnlock() return c.bug.Id() } @@ -41,6 +48,8 @@ func (c *BugCache) notifyUpdated() error { // ResolveOperationWithMetadata will find an operation that has the matching metadata func (c *BugCache) ResolveOperationWithMetadata(key string, value string) (entity.Id, error) { + c.mu.RLock() + defer c.mu.RUnlock() // preallocate but empty matching := make([]entity.Id, 0, 5) @@ -78,6 +87,8 @@ func (c *BugCache) AddCommentWithFiles(message string, files []repository.Hash) } func (c *BugCache) AddCommentRaw(author *IdentityCache, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (*bug.AddCommentOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.AddCommentWithFiles(c.bug, author.Identity, unixTime, message, files) if err != nil { return nil, err @@ -100,6 +111,8 @@ func (c *BugCache) ChangeLabels(added []string, removed []string) ([]bug.LabelCh } func (c *BugCache) ChangeLabelsRaw(author *IdentityCache, unixTime int64, added []string, removed []string, metadata map[string]string) ([]bug.LabelChangeResult, *bug.LabelChangeOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() changes, op, err := bug.ChangeLabels(c.bug, author.Identity, unixTime, added, removed) if err != nil { return changes, nil, err @@ -127,6 +140,8 @@ func (c *BugCache) ForceChangeLabels(added []string, removed []string) (*bug.Lab } func (c *BugCache) ForceChangeLabelsRaw(author *IdentityCache, unixTime int64, added []string, removed []string, metadata map[string]string) (*bug.LabelChangeOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.ForceChangeLabels(c.bug, author.Identity, unixTime, added, removed) if err != nil { return nil, err @@ -154,6 +169,8 @@ func (c *BugCache) Open() (*bug.SetStatusOperation, error) { } func (c *BugCache) OpenRaw(author *IdentityCache, unixTime int64, metadata map[string]string) (*bug.SetStatusOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.Open(c.bug, author.Identity, unixTime) if err != nil { return nil, err @@ -176,6 +193,8 @@ func (c *BugCache) Close() (*bug.SetStatusOperation, error) { } func (c *BugCache) CloseRaw(author *IdentityCache, unixTime int64, metadata map[string]string) (*bug.SetStatusOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.Close(c.bug, author.Identity, unixTime) if err != nil { return nil, err @@ -198,6 +217,8 @@ func (c *BugCache) SetTitle(title string) (*bug.SetTitleOperation, error) { } func (c *BugCache) SetTitleRaw(author *IdentityCache, unixTime int64, title string, metadata map[string]string) (*bug.SetTitleOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.SetTitle(c.bug, author.Identity, unixTime, title) if err != nil { return nil, err @@ -220,6 +241,8 @@ func (c *BugCache) EditCreateComment(body string) (*bug.EditCommentOperation, er } func (c *BugCache) EditCreateCommentRaw(author *IdentityCache, unixTime int64, body string, metadata map[string]string) (*bug.EditCommentOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.EditCreateComment(c.bug, author.Identity, unixTime, body) if err != nil { return nil, err @@ -242,6 +265,8 @@ func (c *BugCache) EditComment(target entity.Id, message string) (*bug.EditComme } func (c *BugCache) EditCommentRaw(author *IdentityCache, unixTime int64, target entity.Id, message string, metadata map[string]string) (*bug.EditCommentOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.EditComment(c.bug, author.Identity, unixTime, target, message) if err != nil { return nil, err @@ -264,6 +289,8 @@ func (c *BugCache) SetMetadata(target entity.Id, newMetadata map[string]string) } func (c *BugCache) SetMetadataRaw(author *IdentityCache, unixTime int64, target entity.Id, newMetadata map[string]string) (*bug.SetMetadataOperation, error) { + c.mu.Lock() + defer c.mu.Unlock() op, err := bug.SetMetadata(c.bug, author.Identity, unixTime, target, newMetadata) if err != nil { return nil, err @@ -273,6 +300,8 @@ func (c *BugCache) SetMetadataRaw(author *IdentityCache, unixTime int64, target } func (c *BugCache) Commit() error { + c.mu.Lock() + defer c.mu.Unlock() err := c.bug.Commit(c.repoCache.repo) if err != nil { return err @@ -281,6 +310,8 @@ func (c *BugCache) Commit() error { } func (c *BugCache) CommitAsNeeded() error { + c.mu.Lock() + defer c.mu.Unlock() err := c.bug.CommitAsNeeded(c.repoCache.repo) if err != nil { return err @@ -289,5 +320,7 @@ func (c *BugCache) CommitAsNeeded() error { } func (c *BugCache) NeedCommit() bool { + c.mu.RLock() + defer c.mu.RUnlock() return c.bug.NeedCommit() } diff --git a/cache/repo_cache_test.go b/cache/repo_cache_test.go index 0deb155e..a85dd4fb 100644 --- a/cache/repo_cache_test.go +++ b/cache/repo_cache_test.go @@ -210,3 +210,21 @@ func TestRemove(t *testing.T) { _, err = repoCache.ResolveBug(b1.Id()) assert.Error(t, bug.ErrBugNotExist, err) } + +//func TestConcurrency(t *testing.T) { +// repo := repository.CreateTestRepo(false) +// defer repository.CleanupTestRepos(repo) +// +// cache, err := NewRepoCache(repo) +// require.NoError(t, err) +// +// iden1, err := cache.NewIdentity("René Descartes", "rene@descartes.fr") +// require.NoError(t, err) +// err = cache.SetUserIdentity(iden1) +// require.NoError(t, err) +// +// bug1, _, err := cache.NewBug("title", "message") +// require.NoError(t, err) +// +// bug1.mu.Lock() +//} -- cgit From c215861e6fe1e696e6515fdade3de1e53a1052f1 Mon Sep 17 00:00:00 2001 From: vince Date: Thu, 13 Aug 2020 20:26:41 +0800 Subject: Remove pointer and unnecessary code --- cache/bug_cache.go | 2 +- cache/repo_cache_test.go | 18 ------------------ 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/cache/bug_cache.go b/cache/bug_cache.go index 01d93593..775af6d1 100644 --- a/cache/bug_cache.go +++ b/cache/bug_cache.go @@ -19,7 +19,7 @@ var ErrNoMatchingOp = fmt.Errorf("no matching operation found") // 3. Deal with concurrency. type BugCache struct { repoCache *RepoCache - mu *sync.RWMutex + mu sync.RWMutex bug *bug.WithSnapshot } diff --git a/cache/repo_cache_test.go b/cache/repo_cache_test.go index a85dd4fb..0deb155e 100644 --- a/cache/repo_cache_test.go +++ b/cache/repo_cache_test.go @@ -210,21 +210,3 @@ func TestRemove(t *testing.T) { _, err = repoCache.ResolveBug(b1.Id()) assert.Error(t, bug.ErrBugNotExist, err) } - -//func TestConcurrency(t *testing.T) { -// repo := repository.CreateTestRepo(false) -// defer repository.CleanupTestRepos(repo) -// -// cache, err := NewRepoCache(repo) -// require.NoError(t, err) -// -// iden1, err := cache.NewIdentity("René Descartes", "rene@descartes.fr") -// require.NoError(t, err) -// err = cache.SetUserIdentity(iden1) -// require.NoError(t, err) -// -// bug1, _, err := cache.NewBug("title", "message") -// require.NoError(t, err) -// -// bug1.mu.Lock() -//} -- cgit From 0fd09aa6b513fc6524ae2aaec147b684341aa72d Mon Sep 17 00:00:00 2001 From: vince Date: Tue, 18 Aug 2020 20:22:04 +0800 Subject: Fix concurrency error --- cache/bug_cache.go | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/cache/bug_cache.go b/cache/bug_cache.go index 775af6d1..8365b3f9 100644 --- a/cache/bug_cache.go +++ b/cache/bug_cache.go @@ -88,9 +88,9 @@ func (c *BugCache) AddCommentWithFiles(message string, files []repository.Hash) func (c *BugCache) AddCommentRaw(author *IdentityCache, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (*bug.AddCommentOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.AddCommentWithFiles(c.bug, author.Identity, unixTime, message, files) if err != nil { + c.mu.Unlock() return nil, err } @@ -98,6 +98,8 @@ func (c *BugCache) AddCommentRaw(author *IdentityCache, unixTime int64, message op.SetMetadata(key, value) } + c.mu.Unlock() + return op, c.notifyUpdated() } @@ -112,9 +114,9 @@ func (c *BugCache) ChangeLabels(added []string, removed []string) ([]bug.LabelCh func (c *BugCache) ChangeLabelsRaw(author *IdentityCache, unixTime int64, added []string, removed []string, metadata map[string]string) ([]bug.LabelChangeResult, *bug.LabelChangeOperation, error) { c.mu.Lock() - defer c.mu.Unlock() changes, op, err := bug.ChangeLabels(c.bug, author.Identity, unixTime, added, removed) if err != nil { + c.mu.Unlock() return changes, nil, err } @@ -122,6 +124,8 @@ func (c *BugCache) ChangeLabelsRaw(author *IdentityCache, unixTime int64, added op.SetMetadata(key, value) } + c.mu.Unlock() + err = c.notifyUpdated() if err != nil { return nil, nil, err @@ -141,9 +145,9 @@ func (c *BugCache) ForceChangeLabels(added []string, removed []string) (*bug.Lab func (c *BugCache) ForceChangeLabelsRaw(author *IdentityCache, unixTime int64, added []string, removed []string, metadata map[string]string) (*bug.LabelChangeOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.ForceChangeLabels(c.bug, author.Identity, unixTime, added, removed) if err != nil { + c.mu.Unlock() return nil, err } @@ -151,6 +155,7 @@ func (c *BugCache) ForceChangeLabelsRaw(author *IdentityCache, unixTime int64, a op.SetMetadata(key, value) } + c.mu.Unlock() err = c.notifyUpdated() if err != nil { return nil, err @@ -170,9 +175,9 @@ func (c *BugCache) Open() (*bug.SetStatusOperation, error) { func (c *BugCache) OpenRaw(author *IdentityCache, unixTime int64, metadata map[string]string) (*bug.SetStatusOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.Open(c.bug, author.Identity, unixTime) if err != nil { + c.mu.Unlock() return nil, err } @@ -180,6 +185,7 @@ func (c *BugCache) OpenRaw(author *IdentityCache, unixTime int64, metadata map[s op.SetMetadata(key, value) } + c.mu.Unlock() return op, c.notifyUpdated() } @@ -194,9 +200,9 @@ func (c *BugCache) Close() (*bug.SetStatusOperation, error) { func (c *BugCache) CloseRaw(author *IdentityCache, unixTime int64, metadata map[string]string) (*bug.SetStatusOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.Close(c.bug, author.Identity, unixTime) if err != nil { + c.mu.Unlock() return nil, err } @@ -204,6 +210,7 @@ func (c *BugCache) CloseRaw(author *IdentityCache, unixTime int64, metadata map[ op.SetMetadata(key, value) } + c.mu.Unlock() return op, c.notifyUpdated() } @@ -218,9 +225,9 @@ func (c *BugCache) SetTitle(title string) (*bug.SetTitleOperation, error) { func (c *BugCache) SetTitleRaw(author *IdentityCache, unixTime int64, title string, metadata map[string]string) (*bug.SetTitleOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.SetTitle(c.bug, author.Identity, unixTime, title) if err != nil { + c.mu.Unlock() return nil, err } @@ -228,6 +235,7 @@ func (c *BugCache) SetTitleRaw(author *IdentityCache, unixTime int64, title stri op.SetMetadata(key, value) } + c.mu.Unlock() return op, c.notifyUpdated() } @@ -242,9 +250,9 @@ func (c *BugCache) EditCreateComment(body string) (*bug.EditCommentOperation, er func (c *BugCache) EditCreateCommentRaw(author *IdentityCache, unixTime int64, body string, metadata map[string]string) (*bug.EditCommentOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.EditCreateComment(c.bug, author.Identity, unixTime, body) if err != nil { + c.mu.Unlock() return nil, err } @@ -252,6 +260,7 @@ func (c *BugCache) EditCreateCommentRaw(author *IdentityCache, unixTime int64, b op.SetMetadata(key, value) } + c.mu.Unlock() return op, c.notifyUpdated() } @@ -266,9 +275,9 @@ func (c *BugCache) EditComment(target entity.Id, message string) (*bug.EditComme func (c *BugCache) EditCommentRaw(author *IdentityCache, unixTime int64, target entity.Id, message string, metadata map[string]string) (*bug.EditCommentOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.EditComment(c.bug, author.Identity, unixTime, target, message) if err != nil { + c.mu.Unlock() return nil, err } @@ -276,6 +285,7 @@ func (c *BugCache) EditCommentRaw(author *IdentityCache, unixTime int64, target op.SetMetadata(key, value) } + c.mu.Unlock() return op, c.notifyUpdated() } @@ -290,32 +300,35 @@ func (c *BugCache) SetMetadata(target entity.Id, newMetadata map[string]string) func (c *BugCache) SetMetadataRaw(author *IdentityCache, unixTime int64, target entity.Id, newMetadata map[string]string) (*bug.SetMetadataOperation, error) { c.mu.Lock() - defer c.mu.Unlock() op, err := bug.SetMetadata(c.bug, author.Identity, unixTime, target, newMetadata) if err != nil { + c.mu.Unlock() return nil, err } + c.mu.Unlock() return op, c.notifyUpdated() } func (c *BugCache) Commit() error { c.mu.Lock() - defer c.mu.Unlock() err := c.bug.Commit(c.repoCache.repo) if err != nil { + c.mu.Unlock() return err } + c.mu.Unlock() return c.notifyUpdated() } func (c *BugCache) CommitAsNeeded() error { c.mu.Lock() - defer c.mu.Unlock() err := c.bug.CommitAsNeeded(c.repoCache.repo) if err != nil { + c.mu.Unlock() return err } + c.mu.Unlock() return c.notifyUpdated() } -- cgit