1

In my Google Apps Script application, there's a place where I generate a unique, incrementing order number. To achieve this, I make use of the builtin LockService and PropertiesService.

I store a number as a script property. When I need a new order number, the application fetches the current value, increments it, and saves the new value for next time. To make sure that two people running the application don't get the same number, access to the script property is placed inside of a script lock, or mutex.

This works well, and hundreds of calls go through the function each day. But twice in the past month, two users have ended up with the same order number.

// Increment the existing value, and return our new value.
function getPropIncrementViaMutex(propId) {
  try {
    var scriptProperties = PropertiesService.getScriptProperties();
    var lock = LockService.getScriptLock();
    var success = false;
    var prevValue = null;
    var newValue = null;
    var wasSet = null;

    while (!success) {
      success = lock.tryLock(500);
      if (!success) {
        Utilities.sleep(1000);
      } else {
        prevValue = Number(scriptProperties.getProperty(propId));
        scriptProperties.setProperty(propId, prevValue + 1);
        newValue  = Number(scriptProperties.getProperty(propId));
        lock.releaseLock();
        wasSet = (newValue === (prevValue + 1));
      }
    }
    if (wasSet) {
      return newValue;
    } else {
      throw new Error("Error incrementing order number. Previous Value: " + prevValue + " New Value: " + newValue);
    }

  } catch(e) {
    if (lock) {
      lock.releaseLock();
    }
    throw e;
  }
}

Is there something I'm doing wrong here? Is the problem on Google's end?

A colleague suggested increasing the lock time in lock.tryLock(500) to something higher like lock.tryLock(800). He also suggests that when releasing the lock, to call Utility.sleep(300) beforehand, so the script has enough time to update the property. He thinks the release is happening before the the property update.

I'm going to try implementing his suggestions, as they can't hurt, but I'd like to hear any other thoughts on the problem.

WhiteHotLoveTiger
  • 2,088
  • 3
  • 30
  • 41
  • Thanks for the suggestions Sandy. Currently, if I fail to obtain the lock with `tryLock`, I wait for one second, and then try again, over and over. I'm still able to know whether the lock worked or not because `tryLock` returns a boolean indicating if the lock was acquired: https://developers.google.com/apps-script/reference/lock/lock#tryLock(Integer) – WhiteHotLoveTiger May 11 '17 at 17:43
  • Personally, I don't like to use PropertiesServices for quick access and writing. Always had issues with, you can try using a spreadsheet to generate your order number. I don't see anything wrong with your code on the use of a lock. However, would like to point out having a serial order number is a flaw/risk (Spoofing risk) in online systems, adding a random string after the order number mitigates the risk and keeps the order number unique – Jack Brown May 11 '17 at 17:57
  • I would agree with you colleague and guess scriptProperties.getProperty would return a cached value. So there might be some time after you release lock when the value is actually not yet written. Extra sleep is a little bit horrible but might help, did it help ? – gaoithe Mar 04 '19 at 14:03
  • I can't recall if it helped much or not, sorry! We ended up moving to a new system shortly after this, where generating unique order numbers was already baked into the lower level parts of the framework. – WhiteHotLoveTiger Mar 04 '19 at 15:16

1 Answers1

2

This is actually a simpler problem than may think.The issue is in the following section of code:

    prevValue = Number(scriptProperties.getProperty(propId));
    scriptProperties.setProperty(propId, prevValue + 1);
    newValue  = Number(scriptProperties.getProperty(propId));

You set the property to prevValue + 1, then immediately assign newValue to that same property. Within your code these two lines execute synchronously, but the setProperty method is essentially asynchronous in nature. In infrequent circumstances, there may be a delay in writing to PropertiesService on Googles Servers. Meanwhile, your code continues executing. This can cause newValue to be assigned before propId has actually been updated in the previous line.

It's a trivial solution to fix this:

function isTimeUp(startTime, milliSeconds){
  var now = new Date();
  return now.getTime() - startTime.getTime() > milliSeconds
}
.
.    
.
prevValue = Number(scriptProperties.getProperty(propId));
newValue = PrevValue + 1;
scriptProperties.setProperty(propId, newValue);
var start = new Date();
// run while loop for a maximum 30000 milliseconds = 30 seconds, to prevent endless loop
while (Number(scriptProperties.getProperty(propId))) != newValue &&
!isTimeUp(start, 30000)){};
.
.
.

Assign newValue directly from prevValue within your code, rather than from the property. No more asynchronous issue.
The while statement and isTimeUp() function are to make sure the property is updated before releasing the lock. That may be a bit of overkill, but you leave the function certain that propId is properly updated before the next user runs it, and the isTimeUp() function ensures no endless loop in the event of an internet glitch writing to PropertiesService (If I wasn't getting lazy towards the end writing this up, I would have also rolled back any effects if the update timed out. But now we're starting to go down the rabbit hole ;)

Miker
  • 36
  • 3