Conditional async callbacks

Question

I'm writing an Electron program which takes a CSV file as input, and does file operations depending on the CSV content and file existence (it's to manage MAME arcade roms).

In order to have a progress bar on the UI side, I have switched the code from fully synchronous (because it was much easier) to asynchronous.

I just cannot find out how to reliably display a message to the user when all the lines in the CSV file are processed, and all the zip files are copied or removed.

Here is a (simplified) sample method:

fs.readFile(file, { 'encoding': 'utf8' }, (err, fileContents) => {
    let fileCsv = csvparse(fileContents);

    let lines = fileCsv.length;
    fileCsv.forEach((line) => {
        lines--;
        let zip = line.name + '.zip';
        let sourceRom = path.join(romset, zip);
        let destRom = path.join(selection, zip);

        this.emit('progress.add', fileCsv.length, fileCsv.length - lines, zip);

        if (fs.existsSync(sourceRom) && !fs.existsSync(destRom)) {
            fs.copy(sourceRom, destRom, (err) => {
                let sourceChd = path.join(romset, game);
                if (fs.existsSync(sourceChd)) {
                    fs.copy(sourceChd, path.join(selection, game), (err) => {
                        if (lines <= 0) { callback(); } // chd is copied
                    });
                } else {
                    if (lines <= 0) { callback(); } // no chd, rom is copied
                }
            });
        } else {
            if (lines <= 0) { callback(); } // no source found or already exists
        }
    });
});

The problem is that the CSV file is processed really fast, but the file are not copied as fast. So it decrements the lines counter to 0, then after each file copy, it finds that it's zero and triggers the callback.

How do I reliably trigger the callback at the end of all these nested callbacks and conditions?

Thanks

Just do the decrement after you actually finished to complete operation (with copying and everything)? — Bergi, Apr 08 '18 at 21:00
Don't ignore errors. You'll want to have a look at promises and `async`/`await`. — Bergi, Apr 08 '18 at 21:01
What do you mean after completing operations? Sometimes I do not do any operation and skip the entry. Sometimes there are 2 file operations, sometimes 1. It's a simplified example, I do not ignore the errors (for now I just rethrow them). I'm looking into promises and all but it's really a mess. — thomasb, Apr 08 '18 at 21:04
Yes, just put the `lines--` at every place where you are finished with an entry, in every place where you have the `if (lines <= 0) { callback(); }`. — Bergi, Apr 08 '18 at 21:05
Just a suggestion, to reduce indentation headaches, you might check for conditions that don't require long blocks afterwards and returning early. You should also use `const` whenever you aren't reassigning a variable. — CertainPerformance, Apr 08 '18 at 21:06
@Bergi: that would change nothing as the problematic one is the one on the bottom, when nothing is processed. — thomasb, Apr 08 '18 at 21:11
@thomasb When nothing is processed, you *do* want to immediately decrement (as you currently do). That's not problematic. The problematic thing is decrementing immediately, then doing asynchronous processing, and only later doing the comparison. Please try it, it will work. — Bergi, Apr 08 '18 at 21:14
Oh! I get it, yeah, my bad, sorry. I'm trying to edit it using promises, if I can't make it work I'll go back and try your solution. Thanks :) — thomasb, Apr 08 '18 at 21:16
Also, it's an antipattern to do "file exists" checks. Filesystems are dynamic. The file might be there now and gone one tick later, or the other way around. The result of `fs.existsSync()` is not worth anything. Drop those checks. Copy the file. Handle the error. If you don't want to copy over an existing file, pass the `fs.constants.COPYFILE_EXCL` flag. — Tomalak, Apr 08 '18 at 21:20
@Tomalak : thanks but it's not a professional program (I do C#), it has evolved from an initial shell script (hence the weirdness), I already spent way more time on it than I wanted to, and it's just an ease-of-life personal program, not business-critical. But [feel free to contribute](https://github.com/cosmo0/retropie-arcade-manager) ;) — thomasb, Apr 08 '18 at 21:38
Possible duplicate of [Callback after all asynchronous forEach callbacks are completed](https://stackoverflow.com/questions/18983138/callback-after-all-asynchronous-foreach-callbacks-are-completed) — thomasb, Apr 08 '18 at 21:54

score 1 · Answer 1 · answered Apr 08 '18 at 21:24

I tried to change the code without massively overwriting your style - assuming there is a reason to avoid things like bluebird, async/await & native Promises, and the async lib.

You need to decrement lines after a line is processed. I pulled the processing logic out into a function to make this clearer:

function processLine({
    sourceRom, destRom, romset, game, callback
}) {
    if (fs.existsSync(sourceRom) && !fs.existsSync(destRom)) {
        fs.copy(sourceRom, destRom, (err) => {
            // *really* should handle this error
            let sourceChd = path.join(romset, game);
            if (fs.existsSync(sourceChd)) {
                fs.copy(sourceChd, path.join(selection, game), (err) => {
                    // *really* should handle this error
                    callback();
                });
            } else {
               callback();
            }
        });
    } else {
        callback() // no source found or already exists
    }
}

fs.readFile(file, { 'encoding': 'utf8' }, (err, fileContents) => {
    let fileCsv = csvparse(fileContents);

    let lines = fileCsv.length;
    fileCsv.forEach((line) => {
        let zip = line.name + '.zip';
        let sourceRom = path.join(romset, zip);
        let destRom = path.join(selection, zip);

        this.emit('progress.add', fileCsv.length, fileCsv.length - lines, zip);

        processLine({ sourceRom, destRom, game, romset, callback: () => {
            lines--;
            if (lines <= 0) {
                callback();
            }
        }})
    });
});

Thanks! No reason for the "style", I'm not much of a JS coder, I come from C#. I switched to Promises (see my close/duplicate vote) but I still don't understand how they work. I discovered async/await tonight while searching for a solution. And I don't want to load more libraries than necessary. — thomasb, Apr 08 '18 at 21:57

Conditional async callbacks

1 Answers1