Send message and wait for receive while using async/await and promises the proper way

Question

I have this working code, but sendAndReceive function looks ugly/smelly to me. Eslint complains about using await inside a loop, and using true as conditional in while loop. Would there be a more elegant way to achieve this? I mean, "blocking" until a response appears in inbox before returning a response. This is necessary because I need to get each response before sending the next message, otherwise the device firmware complains.

const SerialPort = require('serialport');
const ReadLine = require('@serialport/parser-readline');

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

const inbox = [];


// IS THIS FUNCTION OK?
const sendAndReceive = async (message, port) => {
  port.write(message);
  while (true) {
    await sleep(1);
    if (inbox.length > 0) {
      const response = inbox.pop();
      return response;
    }
  }
};


SerialPort.list()
  .then(async portInfos => {
    portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
      .forEach(async portInfo => {
        const port = new SerialPort(portInfo.path).setEncoding('ascii');

        const parser = port.pipe(new ReadLine({
          delimiter: '\r\n',
          encoding: 'ascii',
        }));
        parser.on('data', data => {
          inbox.push(data);
        });

        port.open();

        const serialMessage = api.createReadMessage(SERIAL);
        const batteryMessage = api.createReadMessage(BATTERY);

        for await (const m of [serialMessage, batteryMessage]) {
          const response = await sendAndReceive(m, port);
          console.log(m.trim());
          console.log(response);
          console.log();
        }
      });
  });

Answer 1

The while (true) { await sleep(1); ... is potentially very computationally expensive. Yes, it'd be good to refactor it out if possible.

Response bug Your current implementation looks like it has a bug, or what could very easily become a bug. The inbox array is global, but each portInfo array item will push separate, undistinguished items to the inbox array. That is, each forEach iteration will create a port and open it immediately, and then whichever iteration whose sendAndReceive happens to run its timeout first after the data callback has pushed an item to the array will get the response. Here:

const response = await sendAndReceive(m, port);

Unless you have only a single portInfo, the response received is likely to correspond to a different iteration's m and port.

The easy way to fix this would be to declare a separate inbox array inside each iteration - but the whole approach there deserves some reworking, as you noticed.

Inside the innermost loop that calls sendAndReceive, you might construct Promises for each message being iterated over, and push their resolve function to an array. When the parser responds, if the array has a resolver function, shift it out and call it with the data:

portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
    .forEach(portInfo => {
        const port = new SerialPort(portInfo.path).setEncoding('ascii');
        const parser = port.pipe(new ReadLine({
            delimiter: '\r\n',
            encoding: 'ascii',
        }));

        const resolves = [];
        parser.on('data', data => {
            if (resolves.length) {
                resolves.shift()(data);
            }
        });

        port.open();

        const serialMessage = api.createReadMessage(SERIAL);
        const batteryMessage = api.createReadMessage(BATTERY);
        for (const message of [serialMessage, batteryMessage]) {
            new Promise((resolve) => {
                port.write(message);
                resolves.push(resolve)
            })
                .then((response) => {
                    console.log(m.trim());
                    console.log(response);
                });
        }
    });

The promises are currently dangling, which is usually weird, but in this case, the promises will never reject. If parser might not be able to get a data for a given message, and it exposes an API for that (an error event, maybe?), it would be good to listen for that, so errors can be properly handled instead of ignored.

Note that the above approach calls port.write twice at once in a given iteration *once for each method), instead of waiting for the prior message Promise to resolve first - this will speed up your program a bit, since you're now waiting in parallel, not in serial. If you wanted to do something when both messages are received, you'd use Promise.all and pass into it an array of the Promises instead of using for..of. (If you have to wait in serial, you can await the resolution of each Promise inside the loop)

---

Other things I noticed:

for..await is only for async iterators While you're technically allowed to do

for await (const m of [serialMessage, batteryMessage]) {

it doesn't make sense to use for await, since the array there is just a plain array - it's not something that implements the async iterator interface. It'd be more appropriate to use just for (const m ...

Variable names m isn't so great as a variable name. message is significantly more informative.

Only use async when you need to return a Promise You do:

SerialPort.list()
  .then(async portInfos => {
    portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
      .forEach(async portInfo => {

The .then's callback does not use await directly inside of it, nor does it need to return a Promise, so there's no need for the async keyword - it's just potentially confusing noise.