A hideous bug

Just lost a few hours of my life over this so I figured it will be a good thing to perpetuate a relatively distilled form of the problem in hopes that in the future, it will prevent me from repeating it.

The problem
const fs = require('fs')  
var callbackCalled = 0

readFile('package.json', function(err, data) {  
    callbackCalled++
    console.log(callbackCalled)
    if (err) {
        return console.error(err) 
    }
    throw new Error('something went wrong in this function')
})

function readFile(file, callback) {  
    fs.readFile(file, function(err, content) {
        if (err) {
            return callback(err)
        }
        try {
            callback(null, JSON.parse(content))
        } catch (e) {
            callback(e)
        }
    })
}

The above code takes a filename, read the contents and attempts to parse it as json.

I tried to be more "correct" and instead of letting possible exceptions from JSON.parse() be thrown I caught them and passed them to the user of readFile() via the callback.

Little did I know of the hole I was digging for myself.

So what's broken in this code? The callback is called twice!

Why?

The callback is invoked synchronously. Therefor the try catch in readFile() will also catch any errors thrown from inside the callback, thus ending up calling it again.

The solution

Change the readFile() function to this

function readFile(file, callback) {  
    fs.readFile(file, function(err, content) {
        if (err) {
            return callback(err)
        }
        var error, result
        try {
            result = JSON.parse(content)
        } catch (e) {
            error = e
        }        
        callback(error, result)
    })
}
Comments powered by Disqus