More resilient updating of ExpiringResource

sdk/azcore/CHANGELOG.md

@@ -9,6 +9,7 @@
 ### Bugs Fixed
 
 ### Other Changes
+* `BearerTokenPolicy` is more resilient to transient authentication failures
 
 ## 0.21.0 (2022-01-11)
 

sdk/azcore/internal/shared/expiring_resource.go

@@ -14,7 +14,7 @@ import (
 // AcquireResource abstracts a method for refreshing an expiring resource.
 type AcquireResource func(state interface{}) (newResource interface{}, newExpiration time.Time, err error)
 
-// ExpiringResource is a temporal resource (usually a credential), that requires periodic refreshing.
+// ExpiringResource is a temporal resource (usually a credential) that requires periodic refreshing.
 type ExpiringResource struct {
 	// cond is used to synchronize access to the shared resource embodied by the remaining fields
 	cond *sync.Cond
@@ -28,6 +28,9 @@ type ExpiringResource struct {
 	// expiration indicates when the shared resource expires; it is 0 if the resource was never acquired
 	expiration time.Time
 
+	// lastAttempt indicates when a thread/goroutine last attempted to acquire/update the resource
+	lastAttempt time.Time
+
 	// acquireResource is the callback function that actually acquires the resource
 	acquireResource AcquireResource
 }
@@ -43,13 +46,15 @@ func (er *ExpiringResource) GetResource(state interface{}) (interface{}, error)
 	// If the resource is expiring within this time window, update it eagerly.
 	// This allows other threads/goroutines to keep running by using the not-yet-expired
 	// resource value while one thread/goroutine updates the resource.
-	const window = 2 * time.Minute // This example updates the resource 2 minutes prior to expiration
+	const window = 5 * time.Minute   // This example updates the resource 5 minutes prior to expiration
+	const backoff = 30 * time.Second // Minimum wait time between eager update attempts
 
-	now, acquire, resource := time.Now(), false, er.resource
+	now, acquire, expired, resource := time.Now(), false, false, er.resource
 	// acquire exclusive lock
 	er.cond.L.Lock()
 	for {
-		if er.expiration.IsZero() || er.expiration.Before(now) {
+		expired = er.expiration.IsZero() || er.expiration.Before(now)
+		if expired {
 			// The resource was never acquired or has expired
 			if !er.acquiring {
 				// If another thread/goroutine is not acquiring/updating the resource, this thread/goroutine will do it
@@ -59,8 +64,9 @@ func (er *ExpiringResource) GetResource(state interface{}) (interface{}, error)
 			// Getting here means that this thread/goroutine will wait for the updated resource
 		} else if er.expiration.Add(-window).Before(now) {
 			// The resource is valid but is expiring within the time window
-			if !er.acquiring {
-				// If another thread/goroutine is not acquiring/renewing the resource, this thread/goroutine will do it
+			if !er.acquiring && er.lastAttempt.Add(backoff).Before(now) {
+				// If another thread/goroutine is not acquiring/renewing the resource, and none has attempted
+				// to do so within the last 30 seconds, this thread/goroutine will do it
 				er.acquiring, acquire = true, true
 				break
 			}
@@ -81,15 +87,21 @@ func (er *ExpiringResource) GetResource(state interface{}) (interface{}, error)
 	if acquire {
 		// This thread/goroutine has been selected to acquire/update the resource
 		var expiration time.Time
-		resource, expiration, err = er.acquireResource(state)
+		var newValue interface{}
+		er.lastAttempt = now
+		newValue, expiration, err = er.acquireResource(state)
 
 		// Atomically, update the shared resource's new value & expiration.
 		er.cond.L.Lock()
 		if err == nil {
-			// No error, update resource & expiration
+			// Update resource & expiration, return the new value
+			resource = newValue
 			er.resource, er.expiration = resource, expiration
+		} else if !expired {
+			// An eager update failed. Discard the error and return the current--still valid--resource value
+			err = nil
 		}
-		er.acquiring = false // Indicate that no thread/goroutine is currently acquiring the resrouce
+		er.acquiring = false // Indicate that no thread/goroutine is currently acquiring the resource
 
 		// Wake up any waiting threads/goroutines since there is a resource they can ALL use
 		er.cond.L.Unlock()

sdk/azcore/internal/shared/expiring_resource_test.go

@@ -19,7 +19,7 @@ func TestNewExpiringResource(t *testing.T) {
 		s := state.(string)
 		switch s {
 		case "initial":
-			return "updated", time.Now(), nil
+			return "updated", time.Now().Add(-time.Minute), nil
 		case "updated":
 			return "refreshed", time.Now().Add(1 * time.Hour), nil
 		default:
@@ -38,11 +38,35 @@ func TestNewExpiringResource(t *testing.T) {
 	require.Equal(t, "refreshed", res)
 }
 
-func TestNewExpiringResourceError(t *testing.T) {
+func TestExpiringResourceError(t *testing.T) {
+	expectedState := "expected state"
+	expectedError := "expected error"
+	calls := 0
 	er := NewExpiringResource(func(state interface{}) (newResource interface{}, newExpiration time.Time, err error) {
-		return "", time.Time{}, errors.New("failed")
+		calls += 1
+		if calls == 1 {
+			return expectedState, time.Now().Add(time.Minute), nil
+		} else {
+			return "un" + expectedState, time.Time{}, errors.New(expectedError)
+		}
 	})
-	res, err := er.GetResource("stale")
-	require.Error(t, err)
-	require.Equal(t, "", res)
+	res, err := er.GetResource(expectedState)
+	require.NoError(t, err)
+	require.Equal(t, expectedState, res)
+
+	// When an eager update fails, GetResource should return the prior value and no error.
+	er.lastAttempt = time.Now().Add(-time.Hour)
+	for i := 0; i < 3; i++ {
+		res, err = er.GetResource(res)
+		require.NoError(t, err)
+		require.Equal(t, expectedState, res)
+		// GetResource should wait before trying a second eager update i.e. it shouldn't make a third call in this loop
+		require.Equal(t, 2, calls)
+	}
+
+	// After the resource has expired, GetResource should return any error from updating
+	er.expiration = time.Now().Add(-time.Hour)
+	_, err = er.GetResource(res)
+	require.Error(t, err, expectedError)
+	require.Equal(t, 3, calls)
 }