firebase function, counter takes too long to update-CodePudding

I have implemented a counter variable in my real time database, the users are able to call the function and upon 4 calls it should run some code. The problem is that it may take too long to run and update the counter, so if the counter = 1 and two people call the function at a relative same time both of them will get

datasnapshot.child("counter").val() = 1

and both of them will set

promises.push(admin.database().ref('game/'   matchkey   '/counter').set((counter   1))) = 2

while it should be 3

how can I fix this ? (I guess by either making the update faster or forcing the function to wait for the previous one to end (not sure if possible), or something else that I am missing I am new to firebase & ts)

this is my code:

exports.functionTST = functions.https.onCall(async (data, context) => {
  const promises = [];
  JSON.parse(JSON.stringify(data), (key, value) => {
     parse values 
  });

  //set values
  promises.push(admin.database().ref('game/'   matchkey   '/'...).update({
      val1: val2,
    }))

    //counter
    admin.database().ref('game/'   matchkey).once('value')
      .then(datasnapshot => {
        let counter = datasnapshot.child("counter").val()
        if (counter === null) {
          promises.push(admin.database().ref('game/'   matchkey   '/counter').set(1))
        } else if (counter < 3) {
          promises.push(admin.database().ref('game/'   matchkey   '/counter').set((counter   1)))
        } else if (counter == 3) {
          promises.push(admin.database().ref('game/'   matchkey   '/counter').remove())
          //do other stuff
         }
     });

    Promise.all(promises);
    return null
  }
});

Thanks !!

CodePudding user response：

First, let's fix your indentation.

exports.functionTST = functions.https.onCall(async (data, context) => {
  const promises = [];
  JSON.parse(JSON.stringify(data), (key, value) => {
     // parse values 
  });

  // set values
  promises.push(
    admin.database()
      .ref('game/'   matchkey   '/'...)
      .update({
        val1: val2,
      })
  );

  // counter
  admin.database()
    .ref('game/'   matchkey)
    .once('value')
    .then(datasnapshot => {
      let counter = datasnapshot.child("counter").val()
      if (counter === null) {
        promises.push(admin.database().ref('game/'   matchkey   '/counter').set(1))
      } else if (counter < 3) {
        promises.push(admin.database().ref('game/'   matchkey   '/counter').set((counter   1)))
      } else if (counter == 3) {
        promises.push(admin.database().ref('game/'   matchkey   '/counter').remove())
        // do other stuff
      }
    });

  Promise.all(promises);
  return null
});

From this, we can now see a few issues.

The most notable error, is that you aren't awaiting or returning the Promise chain properly. When you don't wait for the promises to resolve, your function will get severely throttled and network requests (such as updating the database) can be skipped entirely.

Promises.all(promises)
return null; // return no data

should be

await Promises.all(promises) // <-- note addition of await here
return null; // return no data

return Promises.all(promises) // <-- note moving return here
  .then(() => null) // return no data

This next block of code does nothing useful. Were you meant to store the result of JSON.parse into a new variable? Depending on what you are trying to achieve Object.entries(data) might be a better choice.

JSON.parse(JSON.stringify(data), (key, value) => {
  // parse values 
});

This next block of code spawns a floating Promise - you aren't returning it, nor are you storing it into an array like promises - so it will cause inconsistent behaviour (like never updating the counter).

admin.database()
  .ref('game/'   matchkey)
  .once('value')
  .then(/* ... */)

The entirety of the code shown in this block should be replaced by a transaction operation instead to streamline the database updates and ensure that it is properly updated.

// set values
promises.push(
  admin.database()
    .ref('game/'   matchkey   '/'...)
    .update({
      val1: val2,
    })
);

// counter
admin.database()
  .ref('game/'   matchkey)
  .once('value')
  .then(datasnapshot => {
    let counter = datasnapshot.child("counter").val()
    if (counter === null) {
      promises.push(admin.database().ref('game/'   matchkey   '/counter').set(1))
    } else if (counter < 3) {
      promises.push(admin.database().ref('game/'   matchkey   '/counter').set((counter   1)))
    } else if (counter == 3) {
      promises.push(admin.database().ref('game/'   matchkey   '/counter').remove())
      //do other stuff
    }
  });

Promises.all(promises)
return null;

Adapting the transaction example and merging your other logic gives:

//counter
await admin.database() // <-- note addition of await here
  .ref('game/'   matchkey)
  .transaction((matchInfo) => {
    if (matchInfo) {
      // set updated values
      matchInfo[/* The '...' from .ref('game/'   matchkey   '/'...) */].val1 = val2;
      // e.g.
      // matchInfo.round1.val1 = val2;
      // OR
      // matchInfo.round1 = Object.assign(
      //   matchInfo.round1 || {}, // <-- creates the property when it is missing
      //   {
      //     val1: val2
      //   }
      // );

      // handle & update counter
      const counter = matchInfo.counter || 0;
      if (counter < 3) {
        matchInfo.counter = counter   1;
      } else {
        // clear counter and do other stuff
        delete matchInfo.counter;
        // e.g.
        // matchInfo.winner = matchInfo.players[0]
      }
    }
    return matchInfo; // apply changes
  });

return null; // return no data to the caller

After implementing the above block, your function can become something similar to:

exports.functionTST = functions.https.onCall(async (data, context) => {
  // parse request body
  const matchkey = data.matchkey;
  /* ... */

  // update match information
  await admin.database() // <-- note addition of await here
    .ref('game/'   matchkey)
    .transaction((matchInfo) => {
      /* ... */
    })
  
  return null;

To ensure that the function returns appropriate errors, you should surround each part in try-catch blocks and throw an appropriate HttpsError so failures can be handled gracefully by the caller on the client side:

exports.functionTST = functions.https.onCall(async (data, context) => {
  // parse request body
  try {
    const matchkey = data.matchkey;
    /* ... */
  } catch (err) {
    console.log('Unexpected error while handling request data: ', err);
    throw new functions.https.HttpsError('invalid-argument', 'Could not parse request data');
  }

  // update match information
  try {
    await admin.database() // <-- note addition of await here
      .ref('game/'   matchkey)
      .transaction((matchInfo) => {
        /* ... */
      })
  } catch (err) {
    console.log('Unexpected error while updating match information: ', err);
    throw new functions.https.HttpsError('internal', 'Could not update match information');
  }
  
  return null; // return no data to the caller
}

CodePudding user response：

First run after deploy take time because the cold start happening, And consider using transaction to prevent race condition.