nodejs tips

1. Add return for setTimeout call

One of the first thing that nodejs developer gets used to is to deal with callback function.

Like..

function dosomething(input, cb) {
  console.log("doing something with", input)
  //... doing something ...
  cb();
}

dosomething(123, function(err) {
  if(err) throw err;
  console.log("success! now doing another thing..")
});

When dosomething gets more complicated, we often forget to add return when we call cb().

Like..

//BROKEN

function dosomething(input, cb) {
  if(input < 0) {
    console.log("doing something with input<0")
    //... doing something ...
    cb();
  }
  
  console.log("doing something with normal input");
  //... doing another thing ...
  cb();
}

In this case, both “something” and “another thing” gets executed because the first cb() would fall through. This would also lead to double callback calls which could be even more damaging than doing both “something” and “another thing” here.

Fortunately, most nodejs developers are pretty used to this stuff and good at making sure that it calls return after cb() is called.

function dosomething(input, cb) {
  if(input < 0) {
    //... doing something ...
    cb();
    return;
  }
}

Or even..

function dosomething(input, cb) {
  if(input < 0) {
    //... doing something ...
    return cb();
  }
}

It’s returning the value from cb() not because it’s really expecting cb() to return something, but to make it logically clear that calling cb() is the last thing that this function does in case input is less than 0.

So far so good.

Now, there is a similar but somewhat less common pattern using setTimeout to make the function to postpone its actions.

Like..

//BROKEN

function dosomething_when_i_can(cb) {
  if(busy()) {
    console.log("resource is busy.. waiting for a second");
    setTimeout(function() {
      dosomething_when_i_can(cb);
    }, 1000);
  }

  //... do something 
  return cb();
}

This function checks to see if resource is busy, and if it is, it calls setTimeout() to call itself to postpone the execution of this function. Do you spot a mistake here? This function not only fails to delay the action, but it ends up making the same action over and over again.

When setTimeout is used to delay some action, it should almost always be accompanied by return, just like cb() - even if it’s the last thing that the funtion does (you might add things later and don’t realize setTimeout isn’t returning)

Just to be verbose, above function shouldn’ve been written like this

//BROKEN

function dosomething_when_i_can(cb) {
  if(busy()) {
    console.log("resource is busy.. waiting for a second");
    return setTimeout(function() {
      dosomething_when_i_can(cb);
    }, 1000);
  }

  //... do something 
  return cb();
}

If setTimeout isn’t used to delay some action, you still probably want to keep the timeout object so that you can cancel or do something with it later.

var to = setTimeout(stuck_handler, 1000);

//... do something

//finished normally.. so cancel stuck_handler
clearTimeout(to);

If you see a naked setTimeout call without a return or assignment, it should be considered a red flag.

Leave a Comment