Promise inside of a Promise creates an endless loop-CodePudding

I have a Promise setup to make an Http call like so (All code has been aggregated):

  callHttpClient({ method, endpoint, payload }) {
    return new Promise((resolve, reject) => {
      axios({
        method: method,
        url: endpoint,
        data: payload,
      }).then(
        (response) => {
          if (response.status == httpStatusCodes.OK) {
            if (response.data == undefined) {
              reject(response);
            } else {
              logEventMixIn.methods.pushEvent('ApiResponse','Success', true);
              resolve(response);
            }
          }
        },
      );
    });
  },

The true flag on the pushEvent will trigger another call inside of the logEventMixin.js file which will also end up returning a Promise:

pushEvent(category, action, shouldLogToDatabase) {
  var payload = {
    userId: getUserId(),
    sessionKey: getSessionKey(),
    pageName: getPageName(),
    sessionId: getSessionId(),
  };

  if(shouldLogToDatabase) {

  httpHelperMixIn.methods.callEndpoint("https://pushEventEndpoint.com", payload); 
   // ** ^^ This returns a Promise ^^ **

  }

  ....
},

The problem is when I pass my flag as true to this method, I actually end up in an endless loop calling the same API endpoint thousands of times. I think it has something to do with the Promise chain, since if I place another Axios call directly in the logEventMixin.js, I don't get the issue anymore (like so)

pushEvent(category, action, shouldLogToDatabase) {
  var payload = {
    userId: getUserId(),
    sessionKey: getSessionKey(),
    pageName: getPageName(),
    sessionId: getSessionId(),
  };

  if(shouldLogToDatabase) {

  axios({
    method: "POST",
    url: "https://pushEventEndpoint.com",
    data: payload,
  }).then((response) => {
    Promise.resolve(response);
  }

What am I doing wrong?

CodePudding user response：

I think you are not handling the Promise fully, try -

callHttpClient({ method, endpoint, payload }) {
    return new Promise((resolve, reject) => {
      axios({
        method: method,
        url: endpoint,
        data: payload,
      }).then(
        (response) => {
          if (response.status == httpStatusCodes.OK) {
            if (response.data == undefined) {
              reject(response);
            } else {
              logEventMixIn.methods.pushEvent('ApiResponse','Success', true).then(() => {
                 resolve(response); // <!-- you should be resolving only when `pushEvent` resolves
              });
              
            }
          }
        },
      );
    });
  },

and also -

pushEvent(category, action, shouldLogToDatabase) {
  var payload = {
    userId: getUserId(),
    sessionKey: getSessionKey(),
    pageName: getPageName(),
    sessionId: getSessionId(),
  };

  if(shouldLogToDatabase) {
     return httpHelperMixIn.methods.callEndpoint("https://pushEventEndpoint.com", payload); 
   // you need to return the above Promise....
  }

  // also return a Promise here for your other code...
  ....
},

CodePudding user response：

We've determined in comments that the source of the loop is indirect recursion, wherein the making a remote call to log a successful remote call generates a loop. The fix is to not log the logs.

The issues with promise code are mostly stylistic, but important because at minimum they obscure the bugs. The following is roughly equivalent to the OP code, but more concise and stylistically correct...

async callHttpClient({ method, endpoint, payload }) {
  const response = await axios({
    method: method,
    url: endpoint,
    data: payload,
  });
  logEventMixIn.methods.pushEvent('ApiResponse','Success', false); // "fire and forget" don't await, and don't log the log
  return response;
}

async pushEvent(category, action, shouldLogToDatabase) {
  var payload = {
    userId: getUserId(),
    sessionKey: getSessionKey(),
    pageName: getPageName(),
    sessionId: getSessionId(),
  };

  if (shouldLogToDatabase) {
    return httpHelperMixIn.methods.callEndpoint("https://pushEventEndpoint.com", payload); 
  } else {
    // do something else with payload
    return Promise.resolve();
  }
}

Note that it makes no mention of error handling. The OP code doesn't either, but that shortcoming is disguised in the OP code with the presence of a reject here or there.

The better pattern on error handling is to throw when an error is found, and catch when something can be done about it, generally earlier in the call chain.