0

I'm curious about best practice here.

As I know the nature of Node (single process), I understand that responding to the web request won't end the process. So, assuming the return value is irrelevant (UPDATE), why do we need to wait for the promise(s) to finish before returning to the client?

In other words, here is a simple Express route:

router.post('/:perId/settings', auth.check, async (req, res) => {

    try {

        let per_id = req.params.perId;
        let per_first_name = req.body.per_first_name.trim();
        let per_last_name = req.body.per_last_name.trim();
        let per_ea_address = req.body.per_ea_address.trim();
        let per_tel_number = std.num_only(req.body.per_tel_number);

        let promises = [];
        let responses;

        promises.push(db.f.stmt((`UPDATE PER SET PER_DATE_UPDATED = CURRENT_TIMESTAMP, PER_FIRST_NAME = @PER_FIRST_NAME, PER_LAST_NAME = @PER_LAST_NAME WHERE PER_ID = @PER_ID`), [
          {var: 'PER_ID', type: sql.Int, val: per_id},
          {var: 'PER_FIRST_NAME', type: sql.VarChar(25), val: per_first_name},
          {var: 'PER_LAST_NAME', type: sql.VarChar(25), val: per_last_name}]));

        promises.push(db.f.stmt((`UPDATE EA SET EA_DATE_UPDATED = CURRENT_TIMESTAMP, EA_ADDRESS = @EA_ADDRESS WHERE PER_ID = @PER_ID`), [
          {var: 'PER_ID', type: sql.Int, val: per_id},
          {var: 'EA_ADDRESS', type: sql.VarChar(60), val: per_ea_address}]));

        promises.push(db.f.stmt((`UPDATE TEL SET TEL_DATE_UPDATED = CURRENT_TIMESTAMP, TEL_NUMBER = @TEL_NUMBER WHERE PER_ID = @PER_ID`), [
          {var: 'PER_ID', type: sql.Int, val: per_id},
          {var: 'TEL_NUMBER', type: sql.VarChar(10), val: per_tel_number}]));

        promises.push(per.insert_history(per_id, (`Settings updated.`), req.user.PER_ID));

        responses = await Promise.all(promises); promises = []; // is this necessary?

        res.status(200).send(column_value);

    } catch (err) {

        await std.log('Unknown error caught', req.originalUrl, err);

        res.status(500).send(('Unknown error caught in ' + req.originalUrl));

    }

});

Is it necessary to include the line:

responses = await Promise.all(promises);

The process doesn't exit so the promises will continue to run, right? In my testing experiences, as expected, they do continue to run after response to the web request. But, I feel like I'm cheating.

Obviously, I'm simply trying to return to the web request as fast as possible.

As an aside, in the event of failure, my db.f.stmt() function will queue failed statements and auto-retry them after a few seconds. If they continue to fail, they are emailed to me so I can fix them and run them manually.

This is important b/c the queries above can only fail do to system issue as opposed to user error. So, returning w/ an error maybe isn't the best move.

Nick
  • 466
  • 1
  • 6
  • 12
  • 3
    In a typical HTTP request scenario, if you're doing a server-side update you don't have to wait. However, if you want to send a meaningful status to the client about the success/failure of the promises, then you need to wait. Waiting will usually be the right thing to do because you can make the UI "optimistic" (ie. assume success until notified otherwise) to deliver a faster user-experience, getting the best of both worlds. – Ben Aston Oct 06 '20 at 13:41
  • 1
    As a completely separate issue, I don't understand why you have `await Promise.all(promises); promises = [];` <-- the second part, `promises = [];`. What's the point of that? – TKoL Oct 06 '20 at 13:43
  • If you say you don't care about errors and handle them by email, what is the `try`/`catch` good for? It looks like there *are* cases where you care. Btw, if the `req.body` has the wrong shape (missing properties, non-string properties, not being an object at all) that's definitely a 400 client/user error. – Bergi Oct 06 '20 at 13:47
  • @Bergi I would guess he doesn't care about errors from the `db.f.stmt` sections, but wants to log errors when eg `req.body.per_first_name.trim()` fails because `per_first_name` isn't defined, or something like that – TKoL Oct 06 '20 at 13:50
  • @TKoL Maybe, maybe not, that's the question to find out how what cases the code should distinguish. – Bergi Oct 06 '20 at 13:56
  • That's true. If there is a coding error and the SQL statements don't fire, the user needs to be aware. That's why the catch block. Clearly, this is a simplified version of something that is very big and complicated and could fail (like undefined, as mentioned). – Nick Oct 06 '20 at 14:29
  • The promises = []; does nothing in this particular sample. It's good practice for me since I re-use this variable for all promise arrays. I always clear my promises array since code any more complicated than a sample like this will need to re-use it and you never want to be caught leaving an old promise in an array. – Nick Oct 06 '20 at 14:30
  • I completely agree in theory about putting the optimism on the client-side JS and providing a good error message upfront unless it gets changed. In practice, two issues: (1) We use one similar ajax() function and, since most ajax calls will need to wait until response is received to show a message, the standard ajax() waits for response to show a message, and (2) I prefer to have all of my success and error messages in the server-side JS, rather than in client-side JS functions. For example, this one might return "Settings updated" but another might return "Record saved". – Nick Oct 06 '20 at 14:35

0 Answers0