Fix multiple refresh issue

[thanpolas]

This fixes #7, includes commits from PR #8.

A fast track to npm would be appreciated as we're blocked by this.

Cheers

[jpodwys]

I doubt that I can look at this today. Hopefully you'll be able to pin your package.json directly to your fork for now.

[jpodwys]

I merged two other PRs (merging in the order received, one of which was your lint PR). There is now a merge conflict with this PR. Please rebase and let me know when it's ready to look at again. Thanks for your patience.

[thanpolas]

@jpodwys ready

[jpodwys]

I copied the unit test changes from this PR and pasted them into my local master and the extra test passes without any of the code changes to redisCacheModule.js. If the extra test passes in the current master branch, why are these changes necessary? Can you improve the unit test to demonstrate the failure that is present in the current master branch? Or if there's a problem with the code change, please address that too. Thanks!

[thanpolas]

Yes, apparently the linting PR took some of it away... I've updated the test to be async when responding on the refresh callback to better illustrate the issue.

[jpodwys]

Haven't forgotten about this. Hoping to verify and merge today.

[jpodwys]

Thanks for finding this issue! I'm surprised I placed async code inside a for loop...

Your fix and unit test look good except for one thing:

Based on a number of stackoverflow discussions, including this one, it's clear that using Object.keys() in conjunction with a regular for loop is faster than using forEach.

With that in mind, please update the backgroundRefresh function as follows:

function backgroundRefresh(){
  var execute = function(key) {
    var data = refreshKeys[key];
    if(data && data.expiration - Date.now() < self.backgroundRefreshMinTtl) {
      data.refresh(key, function (err, response) {
        if(!err){
          self.set(key, response, data.lifeSpan, data.refresh, noop);
        }
      });
    }
  }
  var keys = Object.keys(refreshKeys);
  for(var i = 0; i < keys.length; i++){
    execute(keys[i]);
  }
}

[thanpolas]

@jpodwys This is a critical issue that effectively renders the library broken since it cannot handle more than two concurrent auto-refresh operations. Are you sure you wanna waste time debating which is faster and which is proper?

I'll do the start. Arrays are iterated using .forEach on ES5, that's the convention that's, the expectation. Microoptimizations just stand in the way of code legibility, which in effect makes the code less understandable, which in turn makes the code less maintainable. I had to spend half an hour towards that goal in my previous PR where i linted and formated the whole codebase.

Now on the specific "optimization" you are suggesting:

 for(var i = 0; i < keys.length; i++){

You are counting the array's length on each loop so the i < keys.length comparison needs to be performed, which effectively makes this routine slower than anything.

Again, this PR is critical to the package's operation, we're for over a week requiring this package from my repository so our business can operate.

Please expedite the fix and if you feel the code needs optimization, there are so many issues with the codebase itself that needs optimizing that an array loop is the least of your concerns. E.g. all the try {} catch() {} statements you have throughout the code, break down any optimizations V8 can offer, which are scales more severe performance penalties that an array iteration.

But again, and last time, this PR is to fix a critical issue with the package that renders it broken. We can deal with optimizations later.

[jpodwys]

You are right that I should have prioritized the bug fix over the performance consideration. Therefore I’ve merged the PR and cut release 1.2.1. Thank you for your contribution.

However, throwing ultimatums around is not the right way to get what you want with open-source devs. You saying “But again, and last time…” is unprofessional. 2.5 days of the week you spent waiting were due to your second PR having a merge conflict and an incomplete unit test. Another 2 days were due to the weekend. The rest of the time was due to me being busy at work, fewer than 24 hours of which was due to my performance recommendation. Furthermore, your project was never broken or blocked during any of that time because you had your own fork to which you could pin as I recommended you do only one hour after you submitted your PR.

I appreciate your input and will prioritize better in the future. Please dispense with ultimatums.

As a last note, V8 automatically caches array.length so i < keys.length is just as fast as otherwise.