Code review fixes

juju · Jan 25, 2022 · 1676091 · 1676091
1 parent a388a1f
commit 1676091
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 9 deletions.
diff --git a/resource/repositories/operations_test.go b/resource/repositories/operations_test.go
@@ -13,11 +13,10 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/juju/charm/v8"
 	charmresource "github.com/juju/charm/v8/resource"
+	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/errors"
-
 	"github.com/juju/juju/charmstore"
 	"github.com/juju/juju/resource"
 	"github.com/juju/juju/resource/repositories"
@@ -117,9 +116,7 @@ func (s *OperationsSuite) TestConcurrentGetResource(c *gc.C) {
 		Repository: er,
 		Name:       "company-icon",
 		CharmID:    repositories.CharmID{URL: charm.MustParseURL("cs:gitlab")},
-		Done: func() {
-			done.Done()
-		},
+		Done:       done.Done,
 	}
 
 	start := sync.WaitGroup{}

diff --git a/resource/resourceadapters/limiter.go b/resource/resourceadapters/limiter.go
@@ -5,6 +5,7 @@ package resourceadapters
 
 import (
 	"sync"
+	"time"
 )
 
 // ResourceDownloadLock is used to limit the number of concurrent
@@ -43,8 +44,9 @@ type resourceDownloadLimiter struct {
 // Acquire implements ResourceDownloadLock.
 func (r *resourceDownloadLimiter) Acquire(appName string) {
 	if r.globalLock != nil {
-		logger.Debugf("acquire global resource download lock, current downloads = %d", len(r.globalLock))
+		start := time.Now()
 		r.globalLock <- struct{}{}
+		logger.Debugf("acquire global resource download lock for %q, took %dms", appName, time.Now().Sub(start)/time.Millisecond)
 	}
 	if r.applicationLimit <= 0 {
 		return
@@ -57,15 +59,15 @@ func (r *resourceDownloadLimiter) Acquire(appName string) {
 		r.applicationLocks[appName] = lock
 	}
 	r.mu.Unlock()
-	logger.Debugf("acquire application resource download lock, current downloads = %d", len(lock))
+	start := time.Now()
 	lock <- struct{}{}
+	logger.Debugf("acquire app resource download lock for %q, took %dms", appName, time.Now().Sub(start)/time.Millisecond)
 }
 
 // Release implements ResourceDownloadLock.
 func (r *resourceDownloadLimiter) Release(appName string) {
 	if r.globalLock != nil {
 		<-r.globalLock
-		logger.Debugf("release global resource download lock, current downloads = %d", len(r.globalLock))
 	}
 	if r.applicationLimit <= 0 {
 		return
@@ -77,7 +79,6 @@ func (r *resourceDownloadLimiter) Release(appName string) {
 	if !ok {
 		return
 	}
-	logger.Debugf("release global resource download lock, current downloads = %d", len(lock))
 	<-lock
 	if len(lock) == 0 {
 		delete(r.applicationLocks, appName)

diff --git a/resource/resourceadapters/limiter_test.go b/resource/resourceadapters/limiter_test.go
@@ -33,17 +33,21 @@ func (s *LimiterSuite) TestNoLimits(c *gc.C) {
 
 	totalAcquiredCount := int32(0)
 	trigger := make(chan struct{})
+	started := sync.WaitGroup{}
 	finished := sync.WaitGroup{}
 	for i := 0; i < totalToAcquire; i++ {
+		started.Add(1)
 		finished.Add(1)
 		go func() {
 			defer finished.Done()
+			started.Done()
 			limiter.Acquire("app1")
 			atomic.AddInt32(&totalAcquiredCount, 1)
 			<-trigger
 			limiter.Release("app1")
 		}()
 	}
+	started.Wait()
 
 	done := make(chan bool)
 	go func() {
@@ -88,17 +92,21 @@ func (s *LimiterSuite) TestGlobalLimit(c *gc.C) {
 
 	totalAcquiredCount := int32(0)
 	trigger := make(chan struct{})
+	started := sync.WaitGroup{}
 	finished := sync.WaitGroup{}
 	for i := 0; i < totalToAcquire; i++ {
+		started.Add(1)
 		finished.Add(1)
 		go func() {
 			defer finished.Done()
+			started.Done()
 			limiter.Acquire("app1")
 			atomic.AddInt32(&totalAcquiredCount, 1)
 			<-trigger
 			limiter.Release("app1")
 		}()
 	}
+	started.Wait()
 
 	done := make(chan bool)
 	go func() {
@@ -152,21 +160,25 @@ func (s *LimiterSuite) TestApplicationLimit(c *gc.C) {
 
 	totalAcquiredCount := int32(0)
 	trigger := make(chan struct{})
+	started := sync.WaitGroup{}
 	finished := sync.WaitGroup{}
 	for i := 0; i < numApplications*totalToAcquirePerApplication; i++ {
+		started.Add(1)
 		finished.Add(1)
 		uuid := "app1"
 		if i >= totalToAcquirePerApplication {
 			uuid = "app2"
 		}
 		go func(uui string) {
 			defer finished.Done()
+			started.Done()
 			limiter.Acquire(uuid)
 			atomic.AddInt32(&totalAcquiredCount, 1)
 			<-trigger
 			limiter.Release(uuid)
 		}(uuid)
 	}
+	started.Wait()
 
 	done := make(chan bool)
 	go func() {
@@ -222,8 +234,10 @@ func (s *LimiterSuite) TestGlobalAndApplicationLimit(c *gc.C) {
 
 	totalAcquiredCount := int32(0)
 	trigger := make(chan struct{})
+	started := sync.WaitGroup{}
 	finished := sync.WaitGroup{}
 	for i := 0; i < numApplications*totalToAcquirePerApplication; i++ {
+		started.Add(1)
 		finished.Add(1)
 		uuid := "app1"
 		if i >= 2*totalToAcquirePerApplication {
@@ -233,12 +247,14 @@ func (s *LimiterSuite) TestGlobalAndApplicationLimit(c *gc.C) {
 		}
 		go func(uui string) {
 			defer finished.Done()
+			started.Done()
 			limiter.Acquire(uuid)
 			atomic.AddInt32(&totalAcquiredCount, 1)
 			<-trigger
 			limiter.Release(uuid)
 		}(uuid)
 	}
+	started.Wait()
 
 	done := make(chan bool)
 	go func() {