Merge pull request #147 from gsuitedevs/locking

Reduce unnecessary usage of LockService

Author: Eric Koleda

src/Service.js

@@ -376,25 +376,37 @@ Service_.prototype.handleCallback = function(callbackRequest) {
  *     otherwise.
  */
 Service_.prototype.hasAccess = function() {
+  var token = this.getToken();
+  if (token && !this.isExpired_(token)) return true; // Token still has access.
+  var canGetToken = (token && this.canRefresh_(token)) ||
+      this.privateKey_ || this.grantType_;
+  if (!canGetToken) return false;
+
   return this.lockable_(function() {
-    var token = this.getToken();
-    if (!token || this.isExpired_(token)) {
-      try {
-        if (token && this.canRefresh_(token)) {
-          this.refresh();
-        } else if (this.privateKey_) {
-          this.exchangeJwt_();
-        } else if (this.grantType_) {
-          this.exchangeGrant_();
-        } else {
-          return false;
-        }
-      } catch (e) {
-        this.lastError_ = e;
+    // Get the token again, bypassing the local memory cache.
+    token = this.getToken(true);
+    // Check to see if the token is no longer missing or expired, as another
+    // execution may have refreshed it while we were waiting for the lock.
+    if (token && !this.isExpired_(token)) return true; // Token now has access.
+    try {
+      if (token && this.canRefresh_(token)) {
+        this.refresh();
+        return true;
+      } else if (this.privateKey_) {
+        this.exchangeJwt_();
+        return true;
+      } else if (this.grantType_) {
+        this.exchangeGrant_();
+        return true;
+      } else {
+        // This should never happen, since canGetToken should have been false
+        // earlier.
         return false;
       }
+    } catch (e) {
+      this.lastError_ = e;
+      return false;
     }
-    return true;
   });
 };
 
@@ -579,10 +591,13 @@ Service_.prototype.saveToken_ = function(token) {
 
 /**
  * Gets the token from the service's property store or cache.
+ * @param {boolean?} optSkipMemoryCheck If true, bypass the local memory cache
+ *     when fetching the token.
  * @return {Object} The token, or null if no token was found.
  */
-Service_.prototype.getToken = function() {
-  return this.getStorage().getValue(null);
+Service_.prototype.getToken = function(optSkipMemoryCheck) {
+  // Gets the stored value under the null key, which is reserved for the token.
+  return this.getStorage().getValue(null, optSkipMemoryCheck);
 };
 
 /**

src/Storage.js

@@ -50,19 +50,23 @@ Storage_.CACHE_NULL_VALUE = '__NULL__';
 /**
  * Gets a stored value.
  * @param {string} key The key.
+ * @param {boolean?} optSkipMemoryCheck Whether to bypass the local memory cache
+ *     when fetching the value (the default is false).
  * @return {*} The stored value.
  */
-Storage_.prototype.getValue = function(key) {
+Storage_.prototype.getValue = function(key, optSkipMemoryCheck) {
   var prefixedKey = this.getPrefixedKey_(key);
   var jsonValue;
   var value;
 
-  // Check memory.
-  if (value = this.memory_[key]) {
-    if (value === Storage_.CACHE_NULL_VALUE) {
-      return null;
+  if (!optSkipMemoryCheck) {
+    // Check in-memory cache.
+    if (value = this.memory_[key]) {
+      if (value === Storage_.CACHE_NULL_VALUE) {
+        return null;
+      }
+      return value;
     }
-    return value;
   }
 
   // Check cache.

test/mocks/lock.js

@@ -12,6 +12,7 @@ var waitingFibers = [];
 var MockLock = function() {
   this.hasLock_ = false;
   this.id = Math.random();
+  this.counter = 0;
 };
 
 MockLock.prototype.waitLock = function(timeoutInMillis) {
@@ -20,6 +21,7 @@ MockLock.prototype.waitLock = function(timeoutInMillis) {
     if (!locked || this.hasLock_) {
       locked = true;
       this.hasLock_ = true;
+      this.counter++;
       return;
     } else {
       waitingFibers.push(Fiber.current);

test/test.js

@@ -97,6 +97,23 @@ describe('Service', function() {
       assert.equal(properties.counter, propertiesStart);
     });
 
+    it('should skip the local memory cache when desired', function() {
+      var properties = new MockProperties();
+      var service = OAuth2.createService('test')
+          .setPropertyStore(properties);
+      var token = {
+        access_token: 'foo'
+      };
+      service.saveToken_(token);
+
+      var newToken = {
+        access_token: 'bar'
+      };
+      properties.setProperty('oauth2.test', JSON.stringify(newToken));
+
+      assert.deepEqual(service.getToken(true), newToken);
+    });
+
     it('should load null tokens from the cache',
         function() {
       var cache = new MockCache();
@@ -212,6 +229,47 @@ describe('Service', function() {
         done();
       });
     });
+
+    it('should not acquire a lock when the token is not expired', function() {
+      var token = {
+        granted_time: (new Date()).getTime(),
+        expires_in: 1000,
+        access_token: 'foo',
+        refresh_token: 'bar'
+      };
+      var lock = new MockLock();
+      var properties = new MockProperties({
+        'oauth2.test': JSON.stringify(token)
+      });
+      var service = OAuth2.createService('test')
+          .setClientId('abc')
+          .setClientSecret('def')
+          .setTokenUrl('http://www.example.com')
+          .setPropertyStore(properties)
+          .setLock(lock);
+      service.hasAccess();
+      assert.equal(lock.counter, 0);
+    });
+
+    it('should not acquire a lock when there is no refresh token', function() {
+      var token = {
+        granted_time: 100,
+        expires_in: 100,
+        access_token: 'foo',
+      };
+      var lock = new MockLock();
+      var properties = new MockProperties({
+        'oauth2.test': JSON.stringify(token)
+      });
+      var service = OAuth2.createService('test')
+          .setClientId('abc')
+          .setClientSecret('def')
+          .setTokenUrl('http://www.example.com')
+          .setPropertyStore(properties)
+          .setLock(lock);
+      service.hasAccess();
+      assert.equal(lock.counter, 0);
+    });
   });
 
   describe('#refresh()', function() {