Use local variables to reduce read locks

accesscontrol/jwks.go

@@ -51,21 +51,23 @@ func NewJWKS(uri string, ttl string, transport http.RoundTripper, confContext co
 }
 
 func (j *JWKS) GetKeys(kid string) ([]JWK, error) {
-	var keys []JWK
+	var (
+		keys []JWK
+		err  error
+	)
 
 	j.mtx.RLock()
-	lKeys := len(j.Keys)
+	allKeys := j.Keys
+	expired := j.hasExpired()
 	j.mtx.RUnlock()
-	if lKeys == 0 || j.hasExpired() {
-		if err := j.Load(); err != nil {
+	if len(allKeys) == 0 || expired {
+		allKeys, err = j.Load()
+		if err != nil {
 			return keys, fmt.Errorf("error loading JWKS: %v", err)
 		}
 	}
 
-	j.mtx.RLock()
-	ks := j.Keys
-	j.mtx.RUnlock()
-	for _, key := range ks {
+	for _, key := range allKeys {
 		if key.KeyID == kid {
 			keys = append(keys, key)
 		}
@@ -87,59 +89,58 @@ func (j *JWKS) GetKey(kid string, alg string, use string) (*JWK, error) {
 	return nil, nil
 }
 
-func (j *JWKS) Load() error {
+func (j *JWKS) Load() ([]JWK, error) {
 	var rawJSON []byte
 
 	if j.file != "" {
 		j, err := reader.ReadFromFile("jwks_url", j.file)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		rawJSON = j
 	} else if j.transport != nil {
 		req, err := http.NewRequest("GET", "", nil)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		ctx := context.WithValue(j.context, request.URLAttribute, j.uri)
 		// TODO which roundtrip name?
 		ctx = context.WithValue(ctx, request.RoundTripName, "jwks")
 		req = req.WithContext(ctx)
 		response, err := j.transport.RoundTrip(req)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		if response.StatusCode != 200 {
-			return fmt.Errorf("status code %d", response.StatusCode)
+			return nil, fmt.Errorf("status code %d", response.StatusCode)
 		}
 
 		defer response.Body.Close()
 
 		body, err := ioutil.ReadAll(response.Body)
 		if err != nil {
-			return fmt.Errorf("error reading JWKS response for %q: %v", j.uri, err)
+			return nil, fmt.Errorf("error reading JWKS response for %q: %v", j.uri, err)
 		}
 		rawJSON = body
 	} else {
-		return fmt.Errorf("jwks: missing both file and request")
+		return nil, fmt.Errorf("jwks: missing both file and request")
 	}
 
 	var jwks JWKS
 	err := json.Unmarshal(rawJSON, &jwks)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	j.mtx.Lock()
+	defer j.mtx.Unlock()
+
 	j.Keys = jwks.Keys
 	j.expiry = time.Now().Unix() + int64(j.ttl.Seconds())
-	j.mtx.Unlock()
 
-	return nil
+	return j.Keys, nil
 }
 
 func (jwks *JWKS) hasExpired() bool {
-	jwks.mtx.RLock()
-	defer jwks.mtx.RUnlock()
 	return time.Now().Unix() > jwks.expiry
 }

accesscontrol/jwks_test.go

@@ -48,7 +48,7 @@ func Test_JWKS_Load(t *testing.T) {
 		t.Run(tt.name, func(subT *testing.T) {
 			jwks, err := ac.NewJWKS("file:"+tt.file, "", nil, nil)
 			helper.Must(err)
-			err = jwks.Load()
+			_, err = jwks.Load()
 			if err != nil && tt.expParsed {
 				subT.Error("no jwks parsed")
 			}
@@ -92,7 +92,7 @@ func Test_JWKS_GetKey(t *testing.T) {
 			helper := test.New(subT)
 			jwks, err := ac.NewJWKS("file:"+tt.file, "", nil, nil)
 			helper.Must(err)
-			err = jwks.Load()
+			_, err = jwks.Load()
 			helper.Must(err)
 			jwk, err := jwks.GetKey(tt.kid, tt.alg, tt.use)
 			if jwk == nil && tt.expFound {