Preventing callbacks from accidentally being called twice

Asynchronous calls like this are extremely common in JavaScript:

fetchFile("example.js", function(fileContents){
    console.log(fileContents)
})

In order to avoid freezing the user interface while the file is loading, fetchFile returns immediately and then when example.js has been loaded the callback is called with the file contents.

Here's how fetchFile could be implemented:

function fetchFile(url, callback){
    // Using Fetch API because it's easy, but you could
    // also make a normal Ajax request
    fetch(url)
    .then(r => r.text())
    .then(function(text){
        callback(text)
   })
}

Let's add some caching, so if we request example.js twice, only one actual network request is made.

var cache = {}
function fetchFile(url, callback){
    if (cache[url]){
        callback(cache[url])
    }

    fetch(url)
    .then(r => r.text())
    .then(function(text){
        cache[url] = text
        callback(text)
   })
}

Done!

Actually, there's a problem with our code. Can you spot it?

...

When we run the example above, and the url is cached, the callback is actually called twice!

It's easy to think of calling the callback as a kind of asynchronous return statement. But execution continues after it, and the HTTP request to fetch example.js happens as if the file wasn't in the cache.

The fix is simple, make sure we return after calling the callback with the cached value:

    if (cache[url]){
        callback(cache[url])
        return
    }

But what can we do to prevent this mistake in the first place? There are two approaches.

Use an else branch even if it's not necessary

We could have avoided the whole problem if we had explicitly put the code that makes the HTTP request into an else branch:

function fetchFile(url, callback){
    if (cache[url]){
        callback(cache[url])
        return
    } else {      
        fetch(url)
        .then(r => r.text())
        .then(function(text){
            cache[url] = text
            callback(text)
       })
    }
}

Now the return statement isn't necessary any more and forgetting to put it in has no negative consequences.

Is there anything wrong with having both the else branch and the return statement? I don't think so. The return statement may be redundant and unnecessary, but it doesn't do any harm.

Wrap the callback so calling it twice throws an error

Another option is to keep track of whether the callback has already been called and throw an error if there's an attempt to call it again.

Checking whether the callback has been called before can be done with an onlyAllowOneCall method:

function fetchFile(url, callback){
      callback = onlyAllowOneCall(callback)

      if (cache[url]){
          callback(cache[url])
      }

      // Fetch logic goes here
}

function onlyAllowOneCall(fn){
     var hasBeenCalled = false;    
     return function(){
          if (hasBeenCalled){
               throw Error("Attempted to call callback twice")
          }
          hasBeenCalled = true;
          return fn.apply(this, arguments)
     }
}

If the function has been called before we'll throw an error, otherwise we call the original function fn.

fn.apply(this, arguments) ensures that our callback is called with the same this value and arguments as the original callback, so onlyAllowOneCall doesn't change the behavior of the callback.

This solution is a bit different from the first one, because it only surfaces the problem rather than fixing it.

We could just ignore any further callback calls after the first one. But then, even if the url is in the cache, the unnecessary HTTP request would still be made.

So, while the functionality of our app won't be affected, ignoring additional calls would hide the cost of unnecessary network requests.