0

I have set a timeout for sending back an error. The problem is that if I need to clear the timeout with clearTimeOut() it does indeed kill it, I can see that as the value of errTimeout's _kill shows true in the debugger. But for some reason node still keeps running the script until the timeOutPeriod is over. I guess it won't really create issues in production because the calling function will receive the returned value. But it still kinda pisses me off that it keeps waiting instead of killing the script.

return new Promise((resolve,reject) => {
    function checkResponse () {
        //creates a timeout that returns an error if data not receieved after the specified time.
        let errTimeout = setTimeout(reject, config.timeOutPeriod);
        //the +1 is there because we originally reduced one, we need to use the physical number now.
        if(layerKeycodes.length !== (rows+1) * (columns+1)){
            //if the array is not complete, check again in 100 ms
            setTimeout(checkResponse, 100);
        } else {
            //clear the error timeout
            clearTimeout(errTimeout);
            //send the layerKeycodes to the calling function
            resolve(layerKeycodes);
        }
  }
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • 3
    Each time you call checkResponse() you are creating a new variable `errTimeout` and the previous one is gone so you can conceivably create many many timers with code shown. A better explanation of exactly what you expect this to would be helpful – charlietfl Jan 04 '22 at 19:59
  • 1
    This code looks like it's just got a wrong design. You're polling variables `layerKeycodes.length`, `rows` and `columns` on a timer. Instead, you should embrace event driven coding and hook into whatever event causes those values to change. So, rather than poll them with a timer, check them when an event occurs that could change their values. So, this shouldn't be using recurring timers at all and shouldn't be polling values. – jfriend00 Jan 04 '22 at 20:59
  • @charlietf1 Oh, that's true.. I missed that, thanks – The pro coder not really xd Jan 04 '22 at 21:05
  • @jfriend00 unfortunately this array is returned from a hardware and the connection to it is done with a package that forces me to request & receive many pieces of data asynchronously. It takes some time to get the response. I have tried many different things but couldn't find a way out besides of editing the actual package which I didn't want to. I ended up creating this function which calculates how many objects to expect and considers the data to be received once that length is reached. It may not be the best practice but I personally couldn't find a better way. Is it that bad? – The pro coder not really xd Jan 04 '22 at 21:12
  • Well, if you show us more about what's going on, in the actual code that modifies these variables and what package it is you're using and where these variables come from, perhaps you can use our collective wisdom to find a better way. Polling variables is generally an anti-pattern in nodejs as you should be using event-driven programming to monitor those variables on some event. Polling timers wastes CPU cycles, is subject to delays from when the actual data is ready and can have the types of side effects you mention in your question (delays due to timers still running). – jfriend00 Jan 04 '22 at 21:20
  • If it's your own code populating these variables (even if in response to some other asynchronous callbacks), then there is definitely a better way. We can help if you show us that code that is modifying these variables. – jfriend00 Jan 04 '22 at 21:36
  • Is this the code that's updating these variables (from your previous question): https://stackoverflow.com/questions/70569721/turning-asynchronous-function-from-a-package-to-sync? If so, do you want this promise returned from that function? Or, how does this logic fit with that function? – jfriend00 Jan 04 '22 at 22:14
  • Can you show the complete code, please, as a [mcve], including the call to `checkPromise`? – Bergi Jan 04 '22 at 22:31

1 Answers1

1

It looks like this code is something you're trying to fit into getLayerKeycodes() from this other question to somehow know when all the data has been received from your keyboard hardware.

I'll illustrate how you can plug into that without using timers. Here's what you started with in that other question:

Your original function

const getLayerKeycodes = (keyboard, layer, rows, columns) => {
    //array that stores all the keycodes according to their order
    let layerKeycodes = [];
    //rows and columns start the count at 0 in low level, so we need to decrease one from the actual number. 
    columns --;
    rows --;
    //loop that asks about all the keycodes in a layer
    const dataReceived = (err, data) => {
        if(err) {
            return err;
        }
        // push the current keycode to the array
        // The keycode is always returned as the fifth object.
        layerKeycodes.push(data[5]);
        console.log(layerKeycodes);
    };  
    for (let r = 0 , c = 0;c <= columns; r ++){
        //callback to fire once data is receieved back from the keyboard.
        if(r > rows){
            c++;
            //r will turn to 0 once the continue fires and the loop executes again 
            r = -1;
            continue;
        }
        //Start listening and call dataReceived when data is received 
        keyboard[0].read(dataReceived);
        //Ask keyboard for information about keycode
        // [always 0 (first byte is ignored),always 0x04 (get_keycode),layer requested,row being checked,column being checked]
        keyboard[0].write([0x01,0x04,layer,r,c]);
    }
    console.log(layerKeycodes);
}

Manually created promise to resolve upon completion of all rows/columns

And, you can incorporate the completion detection code inside of the dataReceived() function without any timers and without reworking much of the rest of your logic like this:

const getLayerKeycodes = (keyboard, layer, rows, columns) => {
    return new Promise((resolve, reject) => {
            //array that stores all the keycodes according to their order
            const layerKeycodes = [];
            const totalCells = rows * columns;
            let abort = false;
            
            //rows and columns start the count at 0 in low level, so we need to decrease one from the actual number.
            columns--;
            rows--;

            // function that gets with keyboard data
            function dataReceived(err, data) => {
                if (err) {
                    abort = true;   // set flag to stop sending more requests
                    reject(err);
                    return;
                }
                // push the current keycode to the array
                // The keycode is always returned as the fifth object.
                layerKeycodes.push(data[5]);
                
                // now see if we're done with all of them
                if (layerKeycodes.length >= totalCells) {
                    resolve(layerKeycodes);
                }
            }

            // loop that asks about all the keycodes in a layer
            for (let r = 0, c = 0; c <= columns; r++) {
                // stop sending more requests if we've already gotten an error
                if (abort) {
                    break;
                }
                //callback to fire once data is receieved back from the keyboard.
                if (r > rows) {
                    c++;
                    //r will turn to 0 once the continue fires and the loop executes again
                    r = -1;
                    continue;
                }
                //Start listening and call dataReceived when data is received
                keyboard[0].read(dataReceived);
                //Ask keyboard for information about keycode
                // [always 0 (first byte is ignored),always 0x04 (get_keycode),layer requested,row being checked,column being checked]
                keyboard[0].write([0x01, 0x04, layer, r, c]);
            }
        }
    }
}

A simplified version by promisifying the read function

And, here's a bit simpler version that promisifies the read function so we can use await on it and then just use an async function and a dual nested for loop for simpler loop mechanics.

const util = require('util');

async function getLayerKeycodes(keyboard, layer, rows, columns) => {
    // promisify the keyboard.read()
    const readKeyboard = util.promisify(keyboard[0].read).bind(keyboard[0]);

    //array that stores all the keycodes according to their order
    const layerKeycodes = [];

    // loop that asks about all the keycodes in a layer
    for (let rowCntr = 0; rowCntr < rows; rowCntr++) {
        for (let colCntr = 0; colCntr < columns; colCntr++) {

            // Start listening and collect the promise
            const readPromise = readKeyboard();
            // Ask keyboard for information about keycode
            // [always 0 (first byte is ignored),always 0x04 (get_keycode),layer requested,row being checked,column being checked]
            keyboard[0].write([0x01, 0x04, layer, rowCntr, colCntr]);

            // wait for data to come in
            const data = await readPromise;

            // push the current keycode to the array
            // The keycode is always returned as the fifth object.
            layerKeycodes.push(data[5]);
        }
    }
    return layerCodes;
}

This also makes sure that we send a write, then wait for the data from that write to come back before we sent the next write which seems like a potentially safer way to handle the hardware. Your original code fires all the writes at once which might work, but it seems like the reads could come back in any order. This guarantees sequential order in the layerCodes array which seems safer (I'm not sure if that matters with this data or not).

Error handling in this last version is somewhat automatically handled by the async function and the promises. If the read returns an error, then the readPromise will automatically reject which will abort our loop and in turn reject the promise that the async function returned. So, we don't have to do the abort checking that the previous function with the manually created promise had.


Now, of course, I don't have the ability to run any of these to test them so it's possible there are some typos somewhere, but hopefully you can work through any of those and see the concept for how these work.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • util really makes promises so clean and easy to work with, in contrast to writing a second callback function to fire. The code works great, and I have learnt about this useful package. Thanks. – The pro coder not really xd Jan 05 '22 at 18:30