skip to Main Content

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 !!

2

Answers


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

    Login or Signup to reply.
  2. 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
    

    or

    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
    }
    
    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search