res.status().send() not working correctly in Promise.all-CodePudding

I am writing a code which check authorization from two different API calls in a promise.all method, in which if any one authorization fails, the respective res.send method would be thrown as an error, but there is an error : Cannot set headers after they are sent to the client error showing on my console, where am I going wrong ?

On the screen, the res.send statement is displayed, but along with that, this error : Cannot set headers after they are sent to the client error showing on my console. How do I fix this ?

I am writing the code in two different ways, but every time the same error is displayed.

1st way ( without .catch ) :

const isSubscribed = new Promise((resolve, reject) => {
  apiGet("/isSubscribed", token).then(async (Response) => {
    if (!isResStatusSubscribed(Response)) return res.status(401).send({ errorMessage: "Unauthorized Request." })
  })
})

const isAdmin = new Promise((resolve, reject) => {
  apiGet("/isAdmin", token).then(async (response) => {
    let isAdmin = response.data.Response.is_admin
    if (!isAdmin) return res.status(403).send({ errorMessage: "User is not an Admin" })
  })
})

Promise.all([isSubscribed, isAdmin]).then(async () => {
  await insertLiveClassDB(req.body)
  return res.status(200).send({ Response: "Success." })
});

2nd way ( with .catch ) :

const isSubscribed = new Promise((resolve, reject) => {
  apiGet("/isSubscribed", token).then(async (Response) => {
    if (!isResStatusSubscribed(Response)) reject(res.status(401).send({ errorMessage: "Unauthorized Request." }))
  })
})

const isAdmin = new Promise((resolve, reject) => {
  apiGet("/isAdmin", token).then(async (response) => {
    let isAdmin = response.data.Response.is_admin
    if (!isAdmin) reject(res.status(403).send({ errorMessage: "User is not an Admin" }))
  })
})

Promise.all([isSubscribed, isAdmin])
.then(async () => {
  await insertLiveClassDB(req.body)
  return res.status(200).send({ Response: "Success." })
})
.catch(error => {
  return error
});

I am new to express js and writing promise.all method, and really need help. Thank you in advance.

CodePudding user response：

There are multiple things going wrong here. First off, you get to send one and only one response to each incoming http request. So, you should never be starting multiple asynchronous operations in parallel and have each of them sending a response. Instead, track the two asynchronous operations with Promise.all() and send the response when the Promise.all() promise finishes.

In addition, you have several examples of the promise constructor anti-pattern where you are wrapping a new promise around a function that already returns a promise. That is considered an anti-pattern for a number of reasons. Not only is unnecessary additional code (you can just directly return the promise you already have), but it's also prone to mistakes in error handling.

Here's what I would suggest:

// stand-alone functions can be declared in a higher scope and
// used by multiple routes
const isSubscribed = function(token) {
    return apiGet("/isSubscribed", token).then((Response) => {
        if (!isResStatusSubscribed(Response)) {
            // turn into a rejection
            let e = new Error("Unauthorized Request");
            e.status = 401;
            throw e;
        }
    });
}

const isAdmin = function(token) {
    return apiGet("/isAdmin", token).then((response) => {
        let isAdmin = response.data.Response.is_admin
        if (!isAdmin) {
            // turn into a rejection
            let e = new Error("User is not an Admin");
            e.status = 403;
            throw e;
        }
    });
}


// code inside your request handler which you already showed to be async
try {
    await Promise.all([isSubscribed(token), isAdmin(token)]);
    await insertLiveClassDB(req.body);
    return res.status(200).send({ Response: "Success." });
} catch(e) {
    let status = e.status || 500;
    return res.status(status).send({errorMessage: e.message});
}

Summary of Changes:

Make isSubscribed() and isAdmin() into a reusable function that returns a promise. That promise resolves if they are subscribed or admin and rejects if not and also rejects if the API had an error.
If those functions get a successful API response, but it shows they aren't subscribed or an admin, then they will reject with a custom Error object that has the message set and a custom property .status set with the proposed response status.
If those functions don't get a successful API response (the API call itself failed), then it will be whatever error object apiGet() rejects with.
Then, await Promise.all([isSubscribed(token), isAdmin(token)]) and if it resolves, then it passed both test. If it rejects, then it failed at least one of the tests and the rejection will be whichever one failed first. You can catch that rejection with a try/catch along with any rejection from insertLiveClassDB(req.body). The catch handler can then send the error response in one place, guaranteeing that you don't attempt to send more than one response.
Note how both isSubscribed() and isAdmin() test their response and turn the returned promise into a reject promise by throw e if the API response indicates failure. This allows the calling code to handle all types of failures in one code path.

CodePudding user response：

If the isSubscribed check fails, there's no point checking for isAdmin, therefore do the checks sequentially.

By throwing errors when the checks fail, then these or any other error, can be handled by a terminal catch.

I would write something like this:

apiGet("/isSubscribed", token)
.then(response => {
    if (!isResStatusSubscribed(response)) {
        throw Object.assign(new Error('Unauthorized Request'), { 'code':401 }); // throw an Error decorated with a 'code' property.
    }
})
.then(() => {
    // arrive here only if the isSubscribed check is successful.
    return apiGet("/isAdmin", token)
    .then(response => {
        if (!response.data.Response.is_admin) {
            throw Object.assign(new Error('User is not an Admin'), { 'code':403 }); // throw an Error decorated with a 'code' property.
        }
    });
})
.then(() => {
    // arrive here only if the isSubscribed and isAdmin checks are successful.
    return insertLiveClassDB(req.body)),
}
.then(() => {
    // arrive here only if the isSubscribed and isAdmin checks and insertLiveClassDB() are successful.
    res.status(200).send({ 'Response': 'Success.' });
})
.catch(e) => {
    // arrive here if anything above has thrown.
    // e.code will be 401, 403 or undefined, depending of where the failure occurred.
    res.status(e.code || 500).send({ 'errorMessage': e.message }); // 500 should be a reasonable default.
};

CodePudding user response：

I don't think there is a need for Promise.all, the performance gain will be marginal. It would be easier to do both checks sequentially and throw an error after each one if needed, or respond once in the end if both conditions pass. But it is doable:

const isSubscribed = async () => {
    const response = await apiGet("/isSubscribed", token);
    if (!isResStatusSubscribed(response)) throw { message : "Unauthorized Request.", status : 401 }; // Will be caught in the Promise.all catch block
}

const isAdmin = async () => {
    const response = await apiGet("/isAdmin", token);
    if (!isResStatusSubscribed(response)) throw { message : "User is not an Admin", status : 403 }; // Will be caught in the Promise.all catch block
}

(async () => {
    try{
        await Promise.all([isSubscribed(), isAdmin()]);
        await insertLiveClassDB(req.body)
        res.status(200).send({ Response: "Success." })
    } catch(err) {
        res.status(err.status).send({ errorMessage : err.message })
    }
})();

Promise.all fails only once, whenever any of the Promises fail. So, the catch block will be triggered only once, even if both conditions throw an error.